Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standalone Processor Server (#884 + #974) #1030

Merged
merged 37 commits into from
Jun 21, 2023
Merged

Standalone Processor Server (#884 + #974) #1030

merged 37 commits into from
Jun 21, 2023

Conversation

MehmedGIT
Copy link
Contributor

@MehmedGIT MehmedGIT commented Mar 29, 2023

This draft PR is supposed to get important aspects from #884 and adapt to #974 so it can extend the ocrd_network package to provide the standalone Processor Server feature.

I am creating this a bit early so it's easy to follow what is going on with this. Still broken and fixes are needed. A lot more refactoring is needed to follow afterward. EDIT: Refactored and on the surface seems to work now.

Feel free to provide feedback.

Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good already.

ocrd_network/ocrd_network/processor_server.py Outdated Show resolved Hide resolved
ocrd_network/ocrd_network/processor_server.py Show resolved Hide resolved
ocrd_network/ocrd_network/processor_server.py Show resolved Hide resolved
ocrd_network/ocrd_network/processor_server.py Show resolved Hide resolved
ocrd/ocrd/decorators/__init__.py Outdated Show resolved Hide resolved
ocrd_network/ocrd_network/processor_server.py Show resolved Hide resolved
ocrd_network/ocrd_network/processor_server.py Outdated Show resolved Hide resolved
ocrd/ocrd/decorators/ocrd_cli_options.py Outdated Show resolved Hide resolved
ocrd_network/ocrd_network/deployer.py Outdated Show resolved Hide resolved
ocrd/ocrd/decorators/ocrd_cli_options.py Outdated Show resolved Hide resolved
@MehmedGIT
Copy link
Contributor Author

MehmedGIT commented Mar 30, 2023

@bertsky

Looks good already.

Not yet. There is still duplication of code. Mainly in the deployment phase.

This is still far away from a review, so please restrain to do so yet.

The reason I said that was to keep #1030 small because most of the things you mentioned in your review would have probably changed in the coming commits. For me going through your review for the current time being and answering everything separately in detail is more time-consuming than actually fixing things. Especially, when things are still not optimized and duplication of code is still out there (due to a clash of implementation ideas of 3 different people which I am trying to combine in #1030). Also, the logging in ocrd_network will change completely after the logging change in the core.

Base automatically changed from dev-processing-broker to master April 2, 2023 12:00
@MehmedGIT
Copy link
Contributor Author

@kba, the blocking API for the Processor Servers and non-blocking requests to the Processor Servers from Processing Server have been implemented. This PR should now be good to be merged.

@MehmedGIT MehmedGIT marked this pull request as ready for review May 8, 2023 14:45
@kba
Copy link
Member

kba commented May 15, 2023

AFAICT the only open discussion is the one around timeouts - shall I move this to a separate issue?

Otherwise, I'll merge after our call later, many thanks!

@bertsky
Copy link
Collaborator

bertsky commented Jun 20, 2023

Now or never!

@kba kba merged commit 2ea5c6b into master Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants