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

Resource resolving not working for $PWD/ocrd-resources #727

Closed
mikegerber opened this issue Oct 18, 2021 · 6 comments
Closed

Resource resolving not working for $PWD/ocrd-resources #727

mikegerber opened this issue Oct 18, 2021 · 6 comments

Comments

@mikegerber
Copy link
Contributor

I have (due to some quick hack to check something) my models in a directory $PWD/ocrd-resources, according to the docs:

ocrd-resources
ocrd-resources/ocrd-calamari-recognize
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/3.ckpt.json
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/4.ckpt.h5
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/1.ckpt.h5
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/0.ckpt.h5
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/2.ckpt.json
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/2.ckpt.h5
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/4.ckpt.json
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/1.ckpt.json
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/0.ckpt.json
ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0/3.ckpt.h5

ocrd-calamari-recognize fails to find these:

15:23:53.051 ERROR ocrd.ocrd-calamari-recognize.resolve_resource - Could not find resource 'qurator-gt4histocr-1.0' for executable 'ocrd-calamari-recognize'. Try 'ocrd resmgr download ocrd-calamari-recognize qurator-gt4histocr-1.0' to download this resource.

I believe that the code at

candidates.append(join(cwd, fname))
should read:

candidates.append(join(cwd, 'ocrd-resources', executable, fname))  # Added 'ocrd-resources', executable

I'm not submitting a PR because I don't know if there are other reasons for leaving out 'ocrd-resources', executable here.

(If I'm putting the models into qurator-gt4histocr-1.0 instead of ocrd-resources/ocrd-calamari-recognize/qurator-gt4histocr-1.0, it works.)

@bertsky
Copy link
Collaborator

bertsky commented Oct 18, 2021

Oh no, I knew this would come 😦

The reason for this design choice was actually backwards compatibility (plus a perceived principle of least astonishment) by expecting relative path names to denote to the CWD directly (without extra implicit prefixes), in the workspace.

It was a conscious decision, open to discussion at the time:

But there was already an adversarial argument: being able to use 'cwd' to dynamically bundle/mount all resources:

Add intermediary directory ocrd-resources to make it easier to mount a host-folder of resources into a Docker image e.g.

This argument lead to a respective change in spec (adding the prefix) and thus docs, but not in core:

I did back this latest change (or was not aware of it) at the time, but personally do not find that use-case too convincing an argument in favour of the prefix: You could always redefine the XDG variables in your Docker/CI setup, and Docker/CI should respect/reflect real-world user demand, not the other way round.

Thus, @kba, since it has never been implemented that way anyhow, would you consider changing the spec back at that spot?

@kba
Copy link
Member

kba commented Oct 26, 2021

Thus, @kba, since it has never been implemented that way anyhow, would you consider changing the spec back at that spot?

Yes, I think we should drop the ocrd-resources prefix for --location cwd from the spec, as there are better ways to get files into Docker as you describe. Sorry for the confusion.

kba added a commit to OCR-D/spec that referenced this issue Oct 28, 2021
@bertsky
Copy link
Collaborator

bertsky commented Nov 10, 2021

@mikegerber does that conclude the case for you?

@mikegerber
Copy link
Contributor Author

@mikegerber does that conclude the case for you?

I think so, but https://ocr-d.de/en/spec/ocrd_tool#file-parameters does not seem up to date yet? (My main issue is that the documentation does not match the behaviour)

@bertsky
Copy link
Collaborator

bertsky commented Nov 11, 2021

I think so, but https://ocr-d.de/en/spec/ocrd_tool#file-parameters does not seem up to date yet? (My main issue is that the documentation does not match the behaviour)

Indeed: that's still the old version. https://github.com/OCR-D/spec/blob/master/ocrd_tool.md#file-parameters has it right, so the website needs to be updated. (And we should probably notify users via Gitter, even if the implemented behaviour has not changed.)

@kba
Copy link
Member

kba commented Nov 24, 2021

I think so, but https://ocr-d.de/en/spec/ocrd_tool#file-parameters does not seem up to date yet? (My main issue is that the documentation does not match the behaviour)

Yes, there was an issue with the publishing workflow for the specs, has been fixed and site has been updated.

(And we should probably notify users via Gitter, even if the implemented behaviour has not changed.)

done

@kba kba closed this as completed Nov 24, 2021
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

No branches or pull requests

3 participants