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.

Sign up for GitHub

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

Remove headless-torch14 and headless-tf2 #314

Merged
merged 2 commits into from
May 4, 2022
Merged

Remove headless-torch14 and headless-tf2 #314

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
babfc8b
Remove unneeded virtual environment headless-torch14
stweil Mar 7, 2022
df2b460
Remove unneeded virtual environment headless-tf2
stweil Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 16 additions & 41 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,8 @@ export VIRTUAL_ENV ?= $(CURDIR)/venv
ifeq (0, $(MAKELEVEL))
SUB_VENV = $(VIRTUAL_ENV)/sub-venv
SUB_VENV_TF1 = $(SUB_VENV)/headless-tf1
SUB_VENV_TF2 = $(SUB_VENV)/headless-tf2
SUB_VENV_TORCH14 = $(SUB_VENV)/headless-torch14
else
SUB_VENV_TF1 = $(VIRTUAL_ENV)
SUB_VENV_TF2 = $(VIRTUAL_ENV)
SUB_VENV_TORCH14 = $(VIRTUAL_ENV)
endif

BIN = $(VIRTUAL_ENV)/bin
Expand Down Expand Up @@ -262,12 +258,12 @@ endif
ifneq ($(findstring ocrd_detectron2, $(OCRD_MODULES)),)
OCRD_EXECUTABLES += $(OCRD_DETECTRON2)
OCRD_DETECTRON2 += $(BIN)/ocrd-detectron2-segment
$(call multirule,$(OCRD_DETECTRON2)): ocrd_detectron2 $(SUB_VENV_TORCH14)/bin/activate
$(call multirule,$(OCRD_DETECTRON2)): ocrd_detectron2 $(SUB_VENV_TF1)/bin/activate
Comment on lines -265 to +261
Copy link
Collaborator

Choose a reason for hiding this comment

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

I fail to see the need to use the TF1 venv for Detectron2, though. If we already have a Pytorch in the main venv, then let's reuse it here. Or did you experience any conflicts with ocrd_typegroups_classifier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The conflict comes from torch, as I have explained above (I updated that comment). The Python module detectron2 is not distributed by PyPI and requires a special torch module which depend on NVIDIA libraries and which we would have to use for all other OCR-D processors, too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's all covered by ocrd_detectron2's makefile though. I can run both modules fine in the same venv. I still don't know what Nvidia libraries you are referring to – please open an issue with ocrd_detectron2, as stated earlier.

Copy link
Collaborator Author

@stweil stweil Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that's an ocrd_detectron2 issue. Extract from https://github.com/stweil/ocrd_all/runs/6178981637:

ImportError: libtorch_cuda_cu.so: cannot open shared object file: No such file or directory
No broken requirements found.
make[1]: Leaving directory '/build'
Makefile:672: recipe for target '/usr/local/bin/ocrd-detectron2-segment-check' failed
make: *** [/usr/local/bin/ocrd-detectron2-segment-check] Error 1

libtorch_cuda_cu.so is only included in the torch module from Facebook Research pytorch.org.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How is that not an ocrd_detectron2 issue?

Perhaps the CUDA version detection failed. It found 11.3.1 earlier, but Pytorch seems to be linked against 10.

libtorch_cuda_cu.so is only included in the torch module from Facebook Research.

Again: our Pytorch is from Pytorch.org or PyPI, not Nvidia or Facebook – you are confusing this.

I also wonder why this does not surface in our CircleCI Docker builds (which use the same core-cuda base image)...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also wonder why this does not surface in our CircleCI Docker builds (which use the same core-cuda base image)...

It simply depends on the order of torch installations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bertsky, I now created an issue which shows the same details as the GitHub action log, see bertsky/ocrd_detectron2#10.

And you are right, the Python torch module was fetched from pytorch.org, not from Facebook Research. But the rest of my conclusion still applies: that version is not compatible with the one from PyPI, and we have either to use it for all OCR-D modules or use a separate venv for ocrd_detectron2 which requires it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: with Pytorch 1.11 out, which is incompatible with Detectron2 v0.6 (built against 1.10.1), you need to update ocrd_detectron2 module once again to avoid runtime failure (std::bad_alloc during import).

I have tested a CPU-only installation with ocrd_detectron2 inside headless-tf1, and it does indeed work. (Also, the other modules there still seem to work.) 🤞

ifeq (0,$(MAKELEVEL))
$(MAKE) -B -o $< $(notdir $(OCRD_DETECTRON2)) VIRTUAL_ENV=$(SUB_VENV_TORCH14)
$(call delegate_venv,$(OCRD_DETECTRON2),$(SUB_VENV_TORCH14))
$(MAKE) -B -o $< $(notdir $(OCRD_DETECTRON2)) VIRTUAL_ENV=$(SUB_VENV_TF1)
$(call delegate_venv,$(OCRD_DETECTRON2),$(SUB_VENV_TF1))
ocrd_detectron2-check:
$(MAKE) check OCRD_MODULES=ocrd_detectron2 VIRTUAL_ENV=$(SUB_VENV_TORCH14)
$(MAKE) check OCRD_MODULES=ocrd_detectron2 VIRTUAL_ENV=$(SUB_VENV_TF1)
else
. $(ACTIVATE_VENV) && $(MAKE) -C $< deps
$(pip_install)
Expand Down Expand Up @@ -432,9 +428,16 @@ OCRD_CIS += $(BIN)/ocrd-cis-ocropy-resegment
OCRD_CIS += $(BIN)/ocrd-cis-ocropy-segment
#OCRD_CIS += $(BIN)/ocrd-cis-ocropy-train
OCRD_CIS += $(BIN)/ocrd-cis-postcorrect
$(call multirule,$(OCRD_CIS)): ocrd_cis $(BIN)/ocrd
$(call multirule,$(OCRD_CIS)): ocrd_cis $(BIN)/ocrd $(SUB_VENV_TF1)/bin/activate
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you pull the current cisocrgroup/ocrd_cis#87, this change is already obsolete.

Moving Ocropy to a sub-venv is something I would avoid at all cost.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That PR is from March 12. I don't want to wait for it with the PR here. We can easily revert the change for ocrd_cis as soon as it is updated in ocrd_all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevertheless, it's the currently recommended version of ocrd_cis, and the same PR is already the basis of ocrd_all master.

I insist: changing venvs unnecessarily must be avoided. Users will likely then have two conflicting versions of ocrd_cis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current version of ocrd_cis in the latest ocrd_all still requires calamari_ocr==0.3.5. Therefore it cannot be in the same virtual environment as ocrd_calamari which is now in the main venv.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current version of ocrd_cis in the latest ocrd_all still requires calamari_ocr==0.3.5. Therefore it cannot be in the same virtual environment as ocrd_calamari which is now in the main venv.

You still don't understand: the latest ocrd_all contains the same (PR) branch of ocrd_cis, but not that very last commit. That's why I said you needed to pull from that PR again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to wait for merges. The whole purpose of the submodule logic is to be able to reference individual commits when needed. Development of ocrd_cis is slow and needs collaboration. Hence the practise of using my feature branches. (This has also been used for core in the past BTW.)

Copy link
Collaborator Author

@stweil stweil Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was speaking of a pull request / merge for ocrd_all. That is required to get your changes for ocrd_cis.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was speaking of a pull request / merge for ocrd_all. That is required to get your changes for ocrd_cis.

Ok, but the whole point was about updating ocrd_cis here in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updating submodules should be done in separate pull requests which usually get merged only by @kba (at least that's how I have understood the workflow). I would like to merge the PR here and can prepare another PR which updates ocrd_cis and puts it back into the main venv later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That does not make sense at all. Placing it in a venv in this PR, only to require making a new PR where it can be removed again.

Once more: I am against placing ocrd_cis into a sub-venv, and there is no need for it.

kba marked this conversation as resolved.
Show resolved Hide resolved
ifeq (0,$(MAKELEVEL))
$(MAKE) -B -o $< $(notdir $(OCRD_CIS)) VIRTUAL_ENV=$(SUB_VENV_TF1)
$(call delegate_venv,$(OCRD_CIS),$(SUB_VENV_TF1))
ocrd_cis-check:
$(MAKE) check OCRD_MODULES=ocrd_cis VIRTUAL_ENV=$(SUB_VENV_TF1)
else
$(pip_install)
endif
endif

ifneq ($(findstring ocrd_pagetopdf, $(OCRD_MODULES)),)
deps-ubuntu-modules: ocrd_pagetopdf
Expand All @@ -451,31 +454,17 @@ install-models-calamari: $(BIN)/ocrd
. $(ACTIVATE_VENV) && ocrd resmgr download ocrd-calamari-recognize '*'
OCRD_EXECUTABLES += $(OCRD_CALAMARI)
OCRD_CALAMARI := $(BIN)/ocrd-calamari-recognize
$(OCRD_CALAMARI): ocrd_calamari $(SUB_VENV_TF2)/bin/activate
ifeq (0,$(MAKELEVEL))
$(MAKE) -B -o $< $(notdir $(OCRD_CALAMARI)) VIRTUAL_ENV=$(SUB_VENV_TF2)
$(call delegate_venv,$(OCRD_CALAMARI),$(SUB_VENV_TF2))
ocrd_calamari-check:
$(MAKE) check OCRD_EXECUTABLES=$(OCRD_CALAMARI) VIRTUAL_ENV=$(SUB_VENV_TF2)
else
$(OCRD_CALAMARI): ocrd_calamari
$(pip_install)
endif
endif

ifneq ($(findstring ocrd_pc_segmentation, $(OCRD_MODULES)),)
OCRD_EXECUTABLES += $(OCRD_PC_SEGMENTATION)
OCRD_PC_SEGMENTATION := $(BIN)/ocrd-pc-segmentation
$(OCRD_PC_SEGMENTATION): ocrd_pc_segmentation $(SUB_VENV_TF2)/bin/activate
ifeq (0,$(MAKELEVEL))
$(MAKE) -B -o $< $(notdir $(OCRD_PC_SEGMENTATION)) VIRTUAL_ENV=$(SUB_VENV_TF2)
$(call delegate_venv,$(OCRD_PC_SEGMENTATION),$(SUB_VENV_TF2))
ocrd_pc_segmentation-check:
$(MAKE) check OCRD_MODULES=ocrd_pc_segmentation VIRTUAL_ENV=$(SUB_VENV_TF2)
else
$(OCRD_PC_SEGMENTATION): ocrd_pc_segmentation
. $(ACTIVATE_VENV) && $(MAKE) -C $< deps
$(pip_install)
endif
endif

ifneq ($(findstring ocrd_anybaseocr, $(OCRD_MODULES)),)
install-models: install-models-anybaseocr
Expand All @@ -493,31 +482,17 @@ OCRD_ANYBASEOCR += $(BIN)/ocrd-anybaseocr-dewarp
OCRD_ANYBASEOCR += $(BIN)/ocrd-anybaseocr-tiseg
OCRD_ANYBASEOCR += $(BIN)/ocrd-anybaseocr-textline
OCRD_ANYBASEOCR += $(BIN)/ocrd-anybaseocr-layout-analysis
$(call multirule,$(OCRD_ANYBASEOCR)): ocrd_anybaseocr $(SUB_VENV_TF2)/bin/activate
ifeq (0,$(MAKELEVEL))
$(MAKE) -B -o $< $(notdir $(OCRD_ANYBASEOCR)) VIRTUAL_ENV=$(SUB_VENV_TF2)
$(call delegate_venv,$(OCRD_ANYBASEOCR),$(SUB_VENV_TF2))
ocrd_anybaseocr-check:
$(MAKE) check OCRD_MODULES=ocrd_anybaseocr VIRTUAL_ENV=$(SUB_VENV_TF2)
else
$(call multirule,$(OCRD_ANYBASEOCR)): ocrd_anybaseocr
$(pip_install)
endif
endif

ifneq ($(findstring ocrd_typegroups_classifier, $(OCRD_MODULES)),)
OCRD_EXECUTABLES += $(OCRD_TYPECLASS)
OCRD_TYPECLASS := $(BIN)/ocrd-typegroups-classifier
OCRD_TYPECLASS += $(BIN)/typegroups-classifier
$(call multirule,$(OCRD_TYPECLASS)): ocrd_typegroups_classifier $(SUB_VENV_TORCH14)/bin/activate
ifeq (0,$(MAKELEVEL))
$(MAKE) -B -o $< $(notdir $(OCRD_TYPECLASS)) VIRTUAL_ENV=$(SUB_VENV_TORCH14)
$(call delegate_venv,$(OCRD_TYPECLASS),$(SUB_VENV_TORCH14))
ocrd_typegroups_classifier-check:
$(MAKE) check OCRD_MODULES=ocrd_typegroups_classifier VIRTUAL_ENV=$(SUB_VENV_TORCH14)
else
$(call multirule,$(OCRD_TYPECLASS)): ocrd_typegroups_classifier
$(pip_install)
endif
endif

ifneq ($(findstring ocrd_doxa, $(OCRD_MODULES)),)
OCRD_EXECUTABLES += $(OCRD_DOXA)
Expand Down