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

Dockerfile update #96

Merged
merged 4 commits into from
Feb 19, 2024
Merged

Dockerfile update #96

merged 4 commits into from
Feb 19, 2024

Conversation

joschrew
Copy link
Contributor

This PR is part of series to offer single ocrd modules as Docker Containers (ocrd slim containers) to be used with ocr-d network.

Dockerfile Outdated
@@ -1,6 +1,6 @@
# Patch and build Olena from Git, then
# Install OCR-D wrapper for binarization
FROM ocrd/core
FROM ocrd/core:v2.62.0 AS base
Copy link
Collaborator

Choose a reason for hiding this comment

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

If similar changes are made for more processors, new releases of OCR-D would require a lot of additional work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In principle – yes. But practically, the opposite is true – without pinning to a specific version, the module maintainers are always forced to react timely to things broken by upstream OCR-D. By pinning, we the maintainer can make a conscious decision on whether and when to update.

Of course, we should at some point do the same thing with pure Python requirements. But for native installation (or ocrd_all via Docker) we currently cannot, because as long as our packages will live in a large unisolated venv, pinning would immediately shrink the intersection of compatible versions to zero. On the other hand, Docker modules are the way to go for greater isolation, and pinning here will now on offer some protection against breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think the added effort of hardcoding a specific version is worth the reproducibility and since it does not concern the deployment with ocrd_all (yet), I see no reason not to.

@joschrew joschrew marked this pull request as draft January 31, 2024 14:32
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.

you also might want to make the same modification (labels) to the build-olena.dockerfile (which is Olena-only, without OCR-D wrapper).

Also, perhaps it's time to make this a single proper multi-stage Dockerfile...

Dockerfile Show resolved Hide resolved
@bertsky bertsky self-requested a review February 12, 2024 13:21
@joschrew joschrew marked this pull request as ready for review February 16, 2024 10:07
@joschrew
Copy link
Contributor Author

Just to confirm, do you think this is ready to be merged? I realized I probably could merge it myself which I would try to do if so. I agree that a multi stage build would be nice. I looked into it but I think I cannot do it and I think this could be regarded as another separate issue. And to me it is not clear what the advantage of the olena-only image is except from size.
Its really nice with the other images like ocrd_tesserocr up to date in docker-hub and the possibility to use that instead of self building ...

@kba
Copy link
Member

kba commented Feb 19, 2024

this could be regarded as another separate issue

#97

Just to confirm, do you think this is ready to be merged? I realized I probably could merge it myself which I would try to do if so.

Yes, I think it's ready for merge and will do so later, thank you!

Dockerfile Outdated Show resolved Hide resolved
@kba kba merged commit 9db42f0 into OCR-D:master Feb 19, 2024
1 check passed
@bertsky
Copy link
Collaborator

bertsky commented Feb 22, 2024

@kba do you have any idea what might be the problem with the Dockerhub deployment?

On CircleCI build-docker, I see

unauthorized: incorrect username or password

But the credentials have not been touched in 9 months, nor has the CI config changed in 2 years...

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.

4 participants