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

refactoring and CLI for presentation #35

Merged
merged 9 commits into from
Oct 18, 2022

Conversation

bertsky
Copy link
Member

@bertsky bertsky commented Sep 28, 2022

This is a first working version for an interface from METS to METS.

We might still need fiddle with the details (both in interface definition and implementation).

(One of the problems I saw was that ocrd_browser cannot show workspaces which still have remote images in them, see here.)

@bertsky
Copy link
Member Author

bertsky commented Sep 28, 2022

Note: for adaptation on the Kitodo.Production side, you will need the following patch:

--- _resources/data/scripts/script_ocr.sh	2022-06-30 15:36:12.000000000 +0200
+++ _resources/data/scripts/script_ocr.sh	2022-09-27 12:23:11.495580732 +0200
@@ -16,11 +16,12 @@
   exit 3
 fi
 
-COMMAND="for_production.sh $PROCESS_ID $TASK_ID /data/$PROCESS_ID deu Fraktur true"
+COMMAND="for_production.sh --proc-id $PROCESS_ID --task-id $TASK_ID --lang deu --script Fraktur"
 WORKFLOW_FILE="ocr-workflow.sh"
 if test -f "/usr/local/kitodo/metadata/$PROCESS_ID/$WORKFLOW_FILE"; then
-    COMMAND+=" /data/$PROCESS_ID/$WORKFLOW_FILE"
+    COMMAND+=" --workflow /data/$PROCESS_ID/$WORKFLOW_FILE"
 fi
+COMMAND+=" /data/$PROCESS_ID"
 
 logger -p user.notice -t $TASK "ssh destination 'ocrd@$OCRD_MANAGER_HOST' port '${OCRD_MANAGER_PORT:-22}' running command '$COMMAND'"
 

(this will then have to go into data.zip again)

Copy link
Collaborator

@markusweigelt markusweigelt left a comment

Choose a reason for hiding this comment

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

This looks very good! I'll then test the changes at least for Production against our stack (with changes from above) and then I'll merge if it works.

In the orcd_lib.sh script I suggest to name functions and values perspectively independent from our applications Kitodo.Production and Kitodo.Presentation and use functional naming. But we could also do this later in a separate issue / PR so that it does not become too much at once.

e.g.
parse_args_for_presentation -> parse_args_for_mets_processing
parse_args_for_production -> parse_args_for_procdir_processing

For closing the task in Kitodo.Production I can imagine a seperated lib kitodo-production_lib.sh. This contains all application-specific functions come.

Afterwards the manager supports Kitodo.Production but the core functionality is independent and can be used with any application.

@bertsky
Copy link
Member Author

bertsky commented Sep 30, 2022

Yes, I thought of doing some of these name changes within this PR already (so we would only need to update the OCR script once). And we could indeed support more use cases than just Kitodo. But then again, we advertise this endeavor as Kitodo integration – why not heed our target users' naming expectations, and stay specific to the purpose? Nobody prevents other users from adapting the shell scripts to their own needs...

@markusweigelt
Copy link
Collaborator

markusweigelt commented Sep 30, 2022

Mainly I am concerned about the naming in the script ocrd_lib.sh. I rather saw this lib as a fixed. So the user does not have to make any adjustments here. I think it would be good to keep the applications out of the lib cause of updateability of the ocrd_lib.sh script when updating OCRD Manager image. So that only adjustments in the scripts which use the library must be made.

@bertsky
Copy link
Member Author

bertsky commented Sep 30, 2022

I see – agreed! Then lets move the arg parsers from ocrd_lib.sh to the top-level scripts.

@bertsky bertsky requested a review from markusweigelt October 4, 2022 12:58
@bertsky
Copy link
Member Author

bertsky commented Oct 18, 2022

Note: the last commit fixes slub/ocrd_kitodo#40

@markusweigelt markusweigelt merged commit da96fab into slub:main Oct 18, 2022
markusweigelt added a commit to slub/ocrd_kitodo that referenced this pull request Oct 19, 2022
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.

2 participants