update, add workflow-configuration, deps-ubuntu and docker #6

bertsky · 2019-11-18T05:42:06Z

@stweil, this is my attempt at a "fat" docker setup. Having difficulties getting versioning right for my slim docker proposal, I think this is at least a good stopgap. The result can be obtained under dockerhub as bertsky/ocrd_all for now (but I hope someone will publish it under ocrd/all soon).

@mikegerber @kba, I also included an updated version of bertsky/workflow-configuration. (For those afraid of calling make I included a system-wide installation as ocrd-make, simply wrapping the make calls.)

stweil · 2019-11-18T06:22:01Z

@kba, should we move the repository to OCR-D? I suggest to keep the name then because I think that just all is too generic.

README.md

stweil · 2019-11-18T06:32:05Z

README.md

+   * `==5.4.1` (required by ocrd_typegroups_classifier)
+   * `>=6.2.0` (required by all others)
+- Tensorflow:
+   * `tensorflow-gpu==1.14.0` (required by ocrd_calamari and OCR-D-LAYoutERkennung)


@mjenckel, would tensorflow-gpu 2.0.0 also work or is a code update to support that possible for OCR-D-LAYoutERkennung?

In this instance, it will be easy for me to hold ocrd_keraslm and cor-asv-ann below 2.0. But the problem will likely re-surface when the first module project wants to go 2.0. (Calamari is already, so ocrd_calamari might follow soon.) The problem then is not only changes in the API (that are rather easy to adapt to) but also that models must be retrained.

BTW, as I explained in #3, there is no tensorflow-gpu for 2.0 (or even 1.15) anymore.

In this instance, it will be easy for me to hold ocrd_keraslm and cor-asv-ann below 2.0.

Done.

bertsky · 2019-11-18T08:16:51Z

@stweil, regarding the pip inconsistencies, as you can see at the bottom of the Dockerfile, I alreaddy added a workaround there (that can be updated as long as no actual incompatibilities are involved).

Do you want me to move that to the Makefile so it is available for the local installation as well?

bertsky · 2019-11-19T10:32:54Z

Do you want me to move that to the Makefile so it is available for the local installation as well?

Done.

@stweil Can you merge?

This does not work in containers, and is not good practise otherwise. (All other modules don't use it.) Plus: we delegate here now in stweil/ocrd_all#6

README.md

stweil

I could not find the updated revision for ocrd_im6convert upstream.

It would be much easier for me to review and merge changes presented in small pull requests which address a single feature and only contain one or two commits.

Makefile

stweil · 2019-11-19T22:09:06Z

@bertsky, I started with cherry-picking changes from this pull request, so you'll have to rebase your branch.

bertsky · 2019-11-19T23:37:00Z

I could not find the updated revision for ocrd_im6convert upstream.

Because it is not merged yet.

It would be much easier for me to review and merge changes presented in small pull requests which address a single feature and only contain one or two commits.

I see. But then those PRs would heavily depend on each other. And some things just belong in one PR. It's been quite difficult making this docker thing work. And now the automatic dependencies for local installation.

@bertsky, I started with cherry-picking changes from this pull request, so you'll have to rebase your branch.

Please don't do this to me.

bertsky · 2019-11-19T23:45:05Z

@stweil When you take this PR apart, are you sure you want to test every single change here with the huge long docker build, which for performance reasons is non-incremental in the commited version? It was already extremely time-consuming having this repeated layer by layer when I made changes. And testing is a must here, you cannot rely on anything...

Dockerfile

Makefile

README.md

kba · 2019-11-20T19:31:06Z

I could not find the updated revision for ocrd_im6convert upstream.

It would be much easier for me to review and merge changes presented in small pull requests which address a single feature and only contain one or two commits.

Yeah, maybe keep the submodule updates out of this PR.

bertsky · 2019-11-20T21:40:42Z

Yeah, maybe keep the submodule updates out of this PR.

I don't understand you guys. Is no-one seeing the connection? Almost all those updates were necessary to get the new functionality (automatic system dependencies, Docker integration, module filtering, workarounds for conflicts) working. In many cases, I conjured up the module updates just for this PR!

With this attitude (keep it small, split it up and rebase), you are making it unnecessarily hard (and frustrating) to bring in solutions. It was already costly to fight through this (waiting for docker build to fail over and over again, wasting tons of network bandwidth, making all this work automatically and maintainably).

I don't want go over all those steps and test them again for no good reason.

You can easily make all / make docker on the PR as a whole – if that fails, we should talk.

stweil · 2019-11-21T08:40:30Z

@bertsky, I now get an ocrd/all docker of 5.02 GB when I run make docker with the latest code which includes most of your modifications. Which tests would you suggest to verify that everything is fine?

Makefile

bertsky · 2019-11-21T12:09:13Z

@bertsky, I now get an ocrd/all docker of 5.02 GB when I run make docker with the latest code which includes most of your modifications. Which tests would you suggest to verify that everything is fine?

Well that's already a (partial) success. (It should be around 7 GB though. Maybe intermediate steps have failed? I recently added set -e to docker.sh to avoid overlooking problems. I'll add that to the PR as soon as the new problem in core is fixed.)

Next steps would be to run some workflow and validate. I am afraid there are no smoke tests for that in core or assets. But you can use workflow-configuration – this is what I do. For example, run

docker run -it -u $(id -u) -v /path/to/workspaces:/data ocrd/all ocrd-make -f gt-binarize-page-olena-sauvola-deskew-page-ocropy-clip-deskew-region-tesseract-resegment-dewarp-ocr-ocropy-tesseract.mk

bertsky · 2019-11-21T12:10:37Z

@stweil Note, there seems to be connectivity problems with GH these days – you might have to repeat steps.

- encapsulate `deps-ubuntu` from modules recursively (including those missing) - add a dockerfile which can include (up to) all executables from all modules without conflicts

- move fix-pip from Dockerfile to Makefile - allow filtering executables by modules they depend on (to easily unselect modules in Docker or pip) - use PIP_INSTALL variable instead of PIP_OPTIONS (and everywhere) - Docker build: increase pip network timeout

- install wget along with deps-ubuntu - don't depend on wget where not needed - document fix-pip and full installation dependencies

…n Dockerfile)

stweil · 2019-11-25T10:46:43Z

You still say non-existing revisions, but as I answered you already, these were perfectly valid revisions, just not on the module's master (yet).

A new revision from a pull request only exists in the repository from which that pull request originated. Such a revision is unavailable for most people.

The rule for a valid submodule revision is simple: the revision must exist in the repository of the submodule (not necessarily in the master branch). If it only exists in your fork, it is not valid.

stweil · 2019-11-25T10:49:13Z

@stweil, I cannot compare this, due to the rebase (that's why I opted for a merge commit instead).

These commands might work for your environment:

git fetch
git diff origin/fat-docker

bertsky · 2019-11-25T10:58:22Z

A new revision from a pull request only exists in the repository from which that pull request originated. Such a revision is unavailable for most people.

That's simply not true: A PR's commits are automatically added to a new branch pull/NUM/head on the receiving end. Did you even try to just git submodule update?

bertsky · 2019-11-25T11:08:24Z

These commands might work for your environment:
git fetch
git diff origin/fat-docker

The updates seem to be ok (but I have not rebuilt and tested the docker images yet). Except for ocrd_im6convert, which definitely needs OCR-D/ocrd_im6convert@1b082f0 (because deps-ubuntu must be run non-interactively), as I said.

Signed-off-by: Stefan Weil <[email protected]>

bertsky · 2019-11-29T10:22:17Z

The updates seem to be ok (but I have not rebuilt and tested the docker images yet). Except for ocrd_im6convert, which definitely needs OCR-D/ocrd_im6convert@1b082f0 (because deps-ubuntu must be run non-interactively), as I said.

I reverted to the needed version, which is now also on ocrd_im6convert:master.

bertsky · 2019-11-29T10:22:57Z

A new revision from a pull request only exists in the repository from which that pull request originated. Such a revision is unavailable for most people.

That's simply not true: A PR's commits are automatically added to a new branch pull/NUM/head on the receiving end. Did you even try to just git submodule update?

With even that out of the way, can you finally merge?

…ively expanded

Makefile

Co-Authored-By: Stefan Weil <[email protected]>

When creating a new venv (because `VIRTUAL_ENV` was _not_ already set in the runtime context of `make`), don't just export `VIRTUAL_ENV`, but force usage of the `activate` script (even for cases of submodules which can hook into existing venvs at install time). Co-Authored-By: Stefan Weil <[email protected]>

Co-Authored-By: Stefan Weil <[email protected]>

bertsky · 2019-11-29T13:39:56Z

@stweil do you want me to update core again here (significantly, we now have processor parameter validation), or are you going to apply that yourself after the PR?

stweil · 2019-11-29T14:03:32Z

I have updated core now.

Co-Authored-By: Stefan Weil <[email protected]>

stweil

We are nearly finished. One last typo is in the way. Thank you for your patience.

README.md

Co-Authored-By: Stefan Weil <[email protected]>

update, add workflow-configuration, deps-ubuntu and docker Signed-off-by: Stefan Weil <[email protected]>

bertsky mentioned this pull request Nov 18, 2019

add information on (prospective) installation/deployment OCR-D/spec#122

Closed

stweil reviewed Nov 18, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

stweil reviewed Nov 18, 2019

View reviewed changes

bertsky mentioned this pull request Nov 19, 2019

make all fails on clean minimal ubuntu eoan #7

Closed

bertsky referenced this pull request in OCR-D/core Nov 19, 2019

Makefile: remove sudo from deps-ubuntu

505101f

This does not work in containers, and is not good practise otherwise. (All other modules don't use it.) Plus: we delegate here now in stweil/ocrd_all#6

stweil reviewed Nov 19, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

stweil requested changes Nov 19, 2019

View reviewed changes

stweil reviewed Nov 19, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

kba approved these changes Nov 20, 2019

View reviewed changes

stweil reviewed Nov 21, 2019

View reviewed changes

Makefile Show resolved Hide resolved

bertsky requested a review from stweil November 22, 2019 13:38

bertsky added 6 commits November 22, 2019 22:45

add deps-ubuntu and (fat) docker for all modules:

bea2a6b

- encapsulate `deps-ubuntu` from modules recursively (including those missing) - add a dockerfile which can include (up to) all executables from all modules without conflicts

Fix deps-ubuntu for vanilla install:

e287ac0

- install wget along with deps-ubuntu - don't depend on wget where not needed - document fix-pip and full installation dependencies

work around core#351 (make install)

b3d8d11

abide by PIP_INSTALL everywhere, allow passing from environment (as i…

23743c7

…n Dockerfile)

deps-ubuntu: tesseract build needs build-essential and automake

7ff4009

bertsky mentioned this pull request Nov 23, 2019

Fat docker container and automatic system dependencies bertsky/ocrd_all#1

Merged

docker-build: use errexit

5088dde

stweil and others added 3 commits November 25, 2019 15:26

Update OCR-D-LAYoutERkennung

6c4fd8d

Signed-off-by: Stefan Weil <[email protected]>

Merge branch 'master' into fat-docker

b52394a

update modules

aa709ea

bertsky requested a review from stweil November 29, 2019 10:25

variables controlling custom install/deps-ubuntu rules must be recurs…

39530a8

…ively expanded