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

make Python 3.8 work #289

Merged
merged 12 commits into from
Mar 18, 2022
Merged

make Python 3.8 work #289

merged 12 commits into from
Mar 18, 2022

Conversation

bertsky
Copy link
Collaborator

@bertsky bertsky commented Feb 19, 2022

Depends on OCR-D/ocrd_anybaseocr#89 (which in turn depends on OCR-D/ocropy#2).

@bertsky
Copy link
Collaborator Author

bertsky commented Feb 19, 2022

CI failure: the same idiotic reason as in the other PRs. Anyone has a solution?

Makefile Show resolved Hide resolved
@kba kba mentioned this pull request Mar 4, 2022
Copy link
Collaborator

@stweil stweil left a comment

Choose a reason for hiding this comment

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

Building with Python 3.7 no longer works for me after I merged this PR. It fails for the headless-tf1 parts.

I also tried Python 3.9 (the default for Debian stable) which failed similarly.

@bertsky
Copy link
Collaborator Author

bertsky commented Mar 4, 2022

Building with Python 3.7 no longer works for me after I merged this PR. It fails for the headless-tf1 parts.

Then please share the error log so we can resolve this quickly.

I also tried Python 3.9 (the default for Debian stable) which failed similarly.

Yeah sure, but that's true for master, too. (I am not claiming I can solve all problems at once.)

@bertsky
Copy link
Collaborator Author

bertsky commented Mar 4, 2022

CI failure:

. /usr/local/sub-venv/headless-torch14/bin/activate && sem --will-cite --fg --id ocrd_all_pip/usr/local/sub-venv/headless-torch14 pip3 install --upgrade pip setuptools
Exception:
Traceback (most recent call last):
  File "/usr/local/sub-venv/headless-torch14/lib/python3.6/site-packages/pip/basecommand.py", line 215, in main
    status = self.run(options, args)
  File "/usr/local/sub-venv/headless-torch14/lib/python3.6/site-packages/pip/commands/install.py", line 290, in run
    with self._build_session(options) as session:
  File "/usr/local/sub-venv/headless-torch14/lib/python3.6/site-packages/pip/basecommand.py", line 69, in _build_session
    if options.cache_dir else None
  File "/usr/lib/python3.6/posixpath.py", line 80, in join
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not int

I remember seeing this before, and solving be replacing/removing the pip cache dir.

@stweil
Copy link
Collaborator

stweil commented Mar 5, 2022

Then please share the error log so we can resolve this quickly.

See https://github.com/stweil/ocrd_all/actions.

Before merging this pull request, Python 3.6 and Python 3.7 work fine while Python 3.8 fails as expected (https://github.com/stweil/ocrd_all/actions/runs/1937666710).

After merging this pull request, Python 3.6 and Python 3.8 work, but Python 3.7 fails (https://github.com/stweil/ocrd_all/actions/runs/1937673668).

@bertsky
Copy link
Collaborator Author

bertsky commented Mar 7, 2022

After merging this pull request, Python 3.6 and Python 3.8 work, but Python 3.7 fails (https://github.com/stweil/ocrd_all/actions/runs/1937673668).

Thanks! I updated the workaround to only activate in the case of Python 3.8. (AFAICS, on 3.7 this is not necessary, and we simply do not newer versions anyway.)

@bertsky
Copy link
Collaborator Author

bertsky commented Mar 7, 2022

I remember seeing this before, and solving be replacing/removing the pip cache dir.

Which is strange, because you would expect the CI container to start without a cache, and without -j there should be no outside interference/race against the cache...

Current CI failure is the 1h timeout again. 😞

@bertsky bertsky requested a review from stweil March 16, 2022 11:54
Copy link
Collaborator

@stweil stweil left a comment

Choose a reason for hiding this comment

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

It looks like handling of non Intel architectures is still missing.

@stweil
Copy link
Collaborator

stweil commented Mar 16, 2022

@bertsky, I think we can address the unhandled target architectures later and apply the pull request.

Maybe you can change the code which checks the Python version and introduce a new macro PYTHON_VERSION which will be needed for other rules, too: stweil@6635444

@bertsky
Copy link
Collaborator Author

bertsky commented Mar 16, 2022

@stweil,

I think we can address the unhandled target architectures later and apply the pull request.

Definitely. When and if...

Maybe you can change the code which checks the Python version and introduce a new macro PYTHON_VERSION which will be needed for other rules, too: stweil@6635444

There's a recipe here already which detects the major version. IIUC you want to expose that as a make variable for other rules as well. How is that related to this PR?

@bertsky
Copy link
Collaborator Author

bertsky commented Mar 16, 2022

CI: 033b5dd does the trick for now. We should revisit the problems related to use of make -j later, but for now the time is just enough for the 1h+1m limit. (We can still disable the lengthy docker image save and artifact uploading later, if we exceed the limit but have not tackled the parallel build issues yet.)

Thus, ready for merging.

@bertsky
Copy link
Collaborator Author

bertsky commented Mar 18, 2022

Now runs into the CircleCI timeout again. I'll disable saving artifacts.

@stweil stweil merged commit 7a761bd into OCR-D:master Mar 18, 2022
Comment on lines +823 to +829
ifneq ($(suffix $(PYTHON)),)
# install specific Python version in system via PPA
apt-get install -y software-properties-common
add-apt-repository -y ppa:deadsnakes/ppa
apt-get update
apt-get install -y --no-install-recommends $(notdir $(PYTHON))-dev $(notdir $(PYTHON))-venv
endif
Copy link
Collaborator

@stweil stweil Mar 30, 2022

Choose a reason for hiding this comment

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

Why was this block added? I don't think that it is a good idea to install a Python package from PPA just because the user added PYTHON=python3.7 to the build options. It can either overwrite an existing installation or install an unneeded 2nd Python installation from a less secure source (which is even added to the apt sources).

I noticed these instructions while building a Docker image with GitLab actions which already provides any desired Python version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because our current reference platform, i.e. the one targeted by deps-ubuntu, is still Ubuntu 18.04. And that uses 3.6 by default for python3, and needs PPAs for more recent versions. (Same with our ocrd_tesserocr recipe.)

I agree it's not a good pattern, but we need some kind of dual version support during the grace period between Ubuntu 18 and 20 (Python 3.6 and 3.8). Think of it as something we can drop as soon as the shift to 20 is complete. (And after that we should find better mechanisms to encapsulate system dependencies.)

I think the natural way to avoid interference with tailored environments like CI is to not set PYTHON there, but instead make it that environment's default version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

18.04 does not need PPA for Python 3.7: https://packages.ubuntu.com/search?keywords=python3.7. And I don't think that the installation of Python should be part of the Makefile rules. Let it fail if someone says make all PYTHON=python3.7 without providing that version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants