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

convert multi-dist to src-package layout #1166

Merged
merged 29 commits into from
Jan 23, 2024
Merged

convert multi-dist to src-package layout #1166

merged 29 commits into from
Jan 23, 2024

Conversation

kba
Copy link
Member

@kba kba commented Jan 19, 2024

After supporting the "multiple dists in one repo" solution in core, it is time to replace it with something easier to manage and more robust to build, which this PR does.

  • move all code of all the packages (ocrd, ocrd_models and so on) into a single source directory src
  • publish only one dist, ocrd to PyPI and GH that contains all the packages that were previously in separate dists
  • port CirlceCI workflow to GH Actions and test for macos/ubuntu{20,22}.04/py3.7-py3.11 and do some preliminary linting

This is a breaking change in how core is distributed but does not require any changes to code.

For the (very few 👋 @bertsky) users who are not using the ocrd dist but one of the ocrd_utils packages, you will need to replace those specific requirements with ocrd, i.e.

# requiremenets.txt
- ocrd_utils >= 2.60.0
+ ocrd >= 2.60.0

We should probably remove those from PyPI altogether?

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.

LGTM

ocrd_network/ocrd_network Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@kba kba force-pushed the single-dist branch 7 times, most recently from 2ffee0f to caef7c5 Compare January 22, 2024 10:53
@kba kba force-pushed the single-dist branch 10 times, most recently from b083308 to fdef922 Compare January 22, 2024 12:20
@bertsky
Copy link
Collaborator

bertsky commented Jan 22, 2024

I would prefer option a) but that would require a new major version, which currently is not ideal since we will be publishing one soon, so we'd have two major versions in the space of a few weeks.

Since I would also prefer option a: Why then does that require a new major? Each release so far had mutual dependencies on the exact same version. The next ocrd release could be simply without any such dependencies. So it's not even a breaking change, unless you depended on ocrd_utils etc. Which we have ruled out already (both textract2page and mets-mods2tei have been updated, just have to re-release on PyPI).

@MehmedGIT
Copy link
Contributor

MehmedGIT commented Jan 23, 2024

The installation from this PR does not work on a fresh Python3.8 environment. I have tested with Python3.7 when reporting yesterday.

Building wheels for collected packages: ocrd, shapely, atomicwrites, sparklines, future
  Building wheel for ocrd (PEP 517) ... done
  Created wheel for ocrd: filename=ocrd-2.60.3-py3-none-any.whl size=329636 sha256=551bd30976a6991676ad3f53c7b14fdba491736cb08285ad3908ca4bd12698e5
  Stored in directory: /tmp/pip-ephem-wheel-cache-sllz5119/wheels/ee/58/fe/1bb59d9b7ef259be5117059606ff84606bc4d36780629c0909
  Building wheel for shapely (PEP 517) ... done
  Created wheel for shapely: filename=shapely-2.0.2-cp38-cp38-linux_x86_64.whl size=1171364 sha256=5630ffac03b3ec597f52d2f515f5f068b433fe7db3d4b0352a7739377eede272
  Stored in directory: /tmp/pip-ephem-wheel-cache-sllz5119/wheels/91/52/55/c71e21531273adfe5f34badcf35a8cdf3439ba140b8c6b52cd
  Building wheel for atomicwrites (setup.py) ... error
  ERROR: Command errored out with exit status 1:
   command: /home/mm/venv38-core/bin/python3.8 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-install-0zfscmxf/atomicwrites/setup.py'"'"'; __file__='"'"'/tmp/pip-install-0zfscmxf/atomicwrites/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-t4c0t38_
       cwd: /tmp/pip-install-0zfscmxf/atomicwrites/
  Complete output (6 lines):
  usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: setup.py --help [cmd1 cmd2 ...]
     or: setup.py --help-commands
     or: setup.py cmd --help
  
  error: invalid command 'bdist_wheel'
  ----------------------------------------
  ERROR: Failed building wheel for atomicwrites
  Running setup.py clean for atomicwrites
  Building wheel for sparklines (setup.py) ... error
  ERROR: Command errored out with exit status 1:
   command: /home/mm/venv38-core/bin/python3.8 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-install-0zfscmxf/sparklines/setup.py'"'"'; __file__='"'"'/tmp/pip-install-0zfscmxf/sparklines/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-_v7k5687
       cwd: /tmp/pip-install-0zfscmxf/sparklines/
  Complete output (6 lines):
  usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: setup.py --help [cmd1 cmd2 ...]
     or: setup.py --help-commands
     or: setup.py cmd --help
  
  error: invalid command 'bdist_wheel'
  ----------------------------------------
  ERROR: Failed building wheel for sparklines
  Running setup.py clean for sparklines
  Building wheel for future (setup.py) ... error
  ERROR: Command errored out with exit status 1:
   command: /home/mm/venv38-core/bin/python3.8 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-install-0zfscmxf/future/setup.py'"'"'; __file__='"'"'/tmp/pip-install-0zfscmxf/future/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-gn7c_96m
       cwd: /tmp/pip-install-0zfscmxf/future/
  Complete output (6 lines):
  usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: setup.py --help [cmd1 cmd2 ...]
     or: setup.py --help-commands
     or: setup.py cmd --help
  
  error: invalid command 'bdist_wheel'
  ----------------------------------------
  ERROR: Failed building wheel for future
  Running setup.py clean for future
Successfully built ocrd shapely
Failed to build atomicwrites sparklines future
ERROR: requests 2.29.0 has requirement urllib3<1.27,>=1.21.1, but you'll have urllib3 2.1.0 which is incompatible.
Installing collected packages: Pillow, python-multipart, h11, typing-extensions, click, uvicorn, numpy, shapely, pydantic, sniffio, idna, certifi, httpcore, exceptiongroup, anyio, httpx, atomicwrites, urllib3, charset-normalizer, requests, packaging, docker, filetype, frozendict, psutil, memory-profiler, wrapt, Deprecated, future, sparklines, bagit, lxml, python-magic, itsdangerous, MarkupSafe, Werkzeug, zipp, importlib-metadata, blinker, Jinja2, Flask, bagit-profile, pyyaml, requests-unixsocket, importlib-resources, attrs, rpds-py, referencing, jsonschema-specifications, pkgutil-resolve-name, jsonschema, pycparser, cffi, pynacl, cryptography, bcrypt, paramiko, pika, starlette, fastapi, toml, lazy-model, dnspython, pymongo, motor, beanie, opencv-python-headless, filelock, tqdm, soupsieve, beautifulsoup4, gdown, ocrd
    Running setup.py install for atomicwrites ... done
    Running setup.py install for future ... done
    Running setup.py install for sparklines ... done
Successfully installed Deprecated-1.2.0 Flask-3.0.1 Jinja2-3.1.3 MarkupSafe-2.1.4 Pillow-10.2.0 Werkzeug-3.0.1 anyio-4.2.0 atomicwrites-1.4.1 attrs-23.2.0 bagit-1.8.1 bagit-profile-1.3.1 bcrypt-4.1.2 beanie-1.24.0 beautifulsoup4-4.12.3 blinker-1.7.0 certifi-2023.11.17 cffi-1.16.0 charset-normalizer-3.3.2 click-8.1.7 cryptography-42.0.0 dnspython-2.5.0 docker-7.0.0 exceptiongroup-1.2.0 fastapi-0.109.0 filelock-3.13.1 filetype-1.2.0 frozendict-2.4.0 future-0.18.3 gdown-5.0.0 h11-0.14.0 httpcore-1.0.2 httpx-0.26.0 idna-3.6 importlib-metadata-7.0.1 importlib-resources-6.1.1 itsdangerous-2.1.2 jsonschema-4.21.1 jsonschema-specifications-2023.12.1 lazy-model-0.2.0 lxml-5.1.0 memory-profiler-0.61.0 motor-3.3.2 numpy-1.24.4 ocrd-2.60.3 opencv-python-headless-4.9.0.80 packaging-23.2 paramiko-3.4.0 pika-1.3.2 pkgutil-resolve-name-1.3.10 psutil-5.9.8 pycparser-2.21 pydantic-1.10.14 pymongo-4.6.1 pynacl-1.5.0 python-magic-0.4.27 python-multipart-0.0.6 pyyaml-6.0.1 referencing-0.32.1 requests-2.29.0 requests-unixsocket-0.3.0 rpds-py-0.17.1 shapely-2.0.2 sniffio-1.3.0 soupsieve-2.5 sparklines-0.4.2 starlette-0.35.1 toml-0.10.2 tqdm-4.66.1 typing-extensions-4.9.0 urllib3-2.1.0 uvicorn-0.27.0 wrapt-1.16.0 zipp-3.17.0
pip config set global.no-binary shapely
Writing to /home/mm/.config/pip/pip.conf

@kba
Copy link
Member Author

kba commented Jan 23, 2024

I would prefer option a) but that would require a new major version, which currently is not ideal since we will be publishing one soon, so we'd have two major versions in the space of a few weeks.

So it's not even a breaking change, unless you depended on ocrd_utils etc. Which we have ruled out already (both textract2page and mets-mods2tei have been updated, just have to re-release on PyPI).

We have ruled out that it is used in PyPI packages. The thing I want to avoid is not breaking installations that do depend on one of the ocrd_* dists and do not realize that a new version exists but that the ocrd_* packages are no longer maintained. But maybe I'm overthinking this.

The installation from this PR does not work on a fresh Python3.8 environment. I have tested with Python3.7 when reporting yesterday.

You're missing the wheel package. pip install wheel should fix that but it should not be necessary to build those packages, e.g. for me, in a fresh 3.8.16 venv, when trying to install those packages without wheel installed, I get

Using legacy 'setup.py install' for atomicwrites, since package 'wheel' is not installed.
Using legacy 'setup.py install' for sparklines, since package 'wheel' is not installed.
Using legacy 'setup.py install' for future, since package 'wheel' is not installed.

What are in your installation

  • python3.8 --version
  • pip --version

@kba
Copy link
Member Author

kba commented Jan 23, 2024

Also, we do not use or need atomicwrites anymore, that one can be removed anyway. Scratch that, we do need it to avoid partial METS writes when save_mets is interrupted.

@MehmedGIT
Copy link
Contributor

What are in your installation

(venv38-core) mm@MM-Notebook:~/repos/core$ python3.8 --version
Python 3.8.10
(venv38-core) mm@MM-Notebook:~/repos/core$ pip --version
pip 20.0.2 from /home/mm/venv38-core/lib/python3.8/site-packages/pip (python 3.8)

@bertsky
Copy link
Collaborator

bertsky commented Jan 23, 2024

Interesting. It does work for me in a fresh venv:

  • Python 3.8.17
  • pip 23.0.1

I merely see warnings for those 3 incriminated packages:

  DEPRECATION: future is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https:/pypa/pip/issues/8559
  Running setup.py install for future ... done
  DEPRECATION: atomicwrites is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https:/pypa/pip/issues/8559
  Running setup.py install for atomicwrites ... done
  DEPRECATION: sparklines is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https:/pypa/pip/issues/8559
  Running setup.py install for sparklines ... done

@bertsky
Copy link
Collaborator

bertsky commented Jan 23, 2024

@MehmedGIT does it work for you if you modify pyproject.toml's build-system.requires to an additional pip>=23.0?

@kba
Copy link
Member Author

kba commented Jan 23, 2024

What are in your installation

(venv38-core) mm@MM-Notebook:~/repos/core$ python3.8 --version
Python 3.8.10
(venv38-core) mm@MM-Notebook:~/repos/core$ pip --version
pip 20.0.2 from /home/mm/venv38-core/lib/python3.8/site-packages/pip (python 3.8)

It's bizarre, I installed 3.8.10 with pyenv and downgraded pip to 20.0.2 and still don't get the error message.

Does pip install -U pip make a difference for you?

Can you try to install a fresh version of 3.8.10 (pyenv install 3.8.10; pyenv local 3.8.10) and see whether that solves it?

@MehmedGIT
Copy link
Contributor

MehmedGIT commented Jan 23, 2024

As @kba suggested, installing the wheel before triggering the ocrd install works without any issues.

Doing pip install -U pip before the ocrd install in a fresh 3.8 env also works without issues.

@MehmedGIT
Copy link
Contributor

MehmedGIT commented Jan 23, 2024

@MehmedGIT does it work for you if you modify pyproject.toml's build-system.requires to an additional pip>=23.0?

Nope, the same error is there. Putting that requirement does not trigger a warning or update confirmation that my pip version is lower than required. Neither auto-updates to the higher version.

@kba
Copy link
Member Author

kba commented Jan 23, 2024

As @kba suggested, installing the wheel before triggering the ocrd install works without any issues.

Sure, but that should happen automatically, wheel is in the build-system.requires. pip should install those before building the package. If this happens to you it might happen to other users.

Any arguments against adding pip install -U pip wheel in the make install recipe? It should not be necessary but better safe than sorry.

BTW, upgrade your pip in any case, newer versions much prettier and slightly faster.

@MehmedGIT
Copy link
Contributor

To conclude, by default, my Python3.8 creates venvs with pip v20.0.2 inside. Installing the wheel or updating the pip to v23 before the ocrd install resolves the issues. I just wonder why the build-system.requires do not auto-update pip.

@bertsky
Copy link
Collaborator

bertsky commented Jan 23, 2024

Any arguments against adding pip install -U pip wheel in the make install recipe? It should not be necessary but better safe than sorry.

Only that we did have situations in the past where we would not easily cope with newer pip (because of some deprecations turned breaks, or arbitrary changes to the dependency resolution algorithm).

Is pyproject.toml's build-system.requires identical to the setup.py's setup.requires BTW?

README: fix links to sub-readmes, move bashlib into separate README
@kba
Copy link
Member Author

kba commented Jan 23, 2024

Is pyproject.toml's build-system.requires identical to the setup.py's setup.requires

Yes, but of course in setup_requires, setuptools is implied but you would specify wheel in there in the past. It is now deprecated in favor of pyproject.toml / build-system.requires.

But what I still don't understand is why wheel is not installed on the fly for @MehmedGIT. If I manually pip uninstall setuptools wheel, both pip install and python -m build work fine for me, installing the build-system.requires in isolation.

Only that we did have situations in the past where we would not easily cope with newer pip (because of some deprecations turned breaks, or arbitrary changes to the dependency resolution algorithm).

True, but I think the danger of people using years-old pip without realizing it is higher than the occasional problematic behavior of the newest versions. Plus we already do that in lots of processor projects.

@kba
Copy link
Member Author

kba commented Jan 23, 2024

I just wonder why the build-system.requires do not auto-update pip.

I might be wrong but I think the idea is that those tools are installed in isolation, just for the installation. So adding pip to build-system.requires does not help because it's not the same pip that is used to invoke the build process.

@kba
Copy link
Member Author

kba commented Jan 23, 2024

True, but I think the danger of people using years-old pip without realizing it is higher than the occasional problematic behavior of the newest versions. Plus we already do that in lots of processor projects.

We also need setuptools at runtime because we use pkg_resources, which we should get rid of asap.

@kba kba merged commit 6f667f7 into master Jan 23, 2024
17 checks passed
@kba kba deleted the single-dist branch June 4, 2024 12:59
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