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

Add pyproject.toml and poetry.lock #20

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jreniel
Copy link

@jreniel jreniel commented Aug 9, 2023

The current setup.py in jigsaw-python uses the module packaging without declaring it as a dependency.
This breaks installation of jigsaw-python using poetry.
The introduction of a pyproject.toml fixes this (PEP 518), allowing installation of jigsaw-python using poetry.

@jreniel
Copy link
Author

jreniel commented Aug 9, 2023

Added poetry-friendly compilation/installation of C-Jigsaw using a poetry run command (in lieu of the compilation found in setup.py).

To invoke the compilation/installation of C-Jigsaw into jigsaw-python source installation, you can do:

poetry run install-jigsaw

@jreniel
Copy link
Author

jreniel commented Aug 9, 2023

The full workflow (assuming you have poetry installed):

git clone https://github.com/dengwirda/jigsaw-python.git # currently should clone this branch for this to work
cd jigsaw-python
poetry shell
poetry install
poetry run install-jigsaw
python -c "import jigsawpy" # verify no library loading errors

In principle, this should be able to reproduce the functionality
of the current setup.py into a poetry environment.
@jreniel
Copy link
Author

jreniel commented Aug 9, 2023

Squashed changes into a single commit.

@jreniel
Copy link
Author

jreniel commented Aug 9, 2023

It is worth nothing that with this method, the wheels include the compiled _lib and _bin, which means that now you can publish these compiled binaries to PyPi along with jigsawpy and it won't require a separate installation. To create the wheels, just do poetry build, which will create a dist/ directory with the wheel files, and if you unzip the wheel file, you will notice it now includes the compiled binaries, which means that the end users can install through pip and won't need to compile the libraries themselves (the binaries are bundled in the wheel).

@dengwirda
Copy link
Owner

Just getting back to this at the end of the year @jreniel --- thanks for putting this interface to poetry together.
I'm not a poetry user myself though, so have some questions to start:

  • Is committing the specific poetry.lock file the right thing to do? It looks like it contains pointers to particular python, numpy, scipy, etc versions, which feels not what I'd prefer in the main jigsawpy repo. Overall, the aim is for jigsawpy to build for an arbitrary python3.x.y using whatever numpy-latest and scipy-latest is at the time.
  • Presently it looks like python itself is sticking with pip, setup.py, etc --- is poetry expected to become a default for python in the future, or is it more of a 3rd-party alternative?
  • It does look like I need to include packaging in the setup.py install_requires list --- it's currently only in requirements.txt (which is usually what I use to build + install jigsawpy).

@jreniel
Copy link
Author

jreniel commented Dec 27, 2023

Hi @dengwirda, those are very good questions. Let's see if I can address them thoroughly.

Is committing the specific poetry.lock file the right thing to do?

Committing a poetry.lock file is recommended, although not strictly necessary. What it does is provide a working deterministic build. However, that doesn't mean this is the only way to build. I'd say lock files are more of a strong suggestion for the compiler, but doesn't mean that whatever is in there is the only way to build. It only tells the compiler which version did work at some point.

It looks like it contains pointers to particular python, numpy, scipy, etc versions, which feels not what I'd prefer in the main jigsawpy repo.

It's not a strict lock, it's more like a strong suggestion for the compiler for a build that did work, but you can build with other versions too.

Overall, the aim is for jigsawpy to build for an arbitrary python3.x.y using whatever numpy-latest and scipy-latest is at the time.

Well, it doesn't always compile with arbitrary versions, been testing this a lot, and I can provide more comments on this later. In fact, if you do test poetry, you'll notice what I'm talking about, but let's build up the knowledge about poetry first and let the system itself guide us.

If you look at the pyprojet.toml I put:
python = ">=3.9.0,<3.13"
That's because some of the dependencies won't work with python<3.9 .... You can test this by changing the python version on this line and trying to build again, poetry will let you know exactly what's wrong. My main suggestion is that you give it a try.

Presently it looks like python itself is sticking with pip, setup.py, etc --- is poetry expected to become a default for python in the future, or is it more of a 3rd-party alternative?

This is a rather complex question. setup.py is getting deprecated for real in favor of pyproject.toml (this is extremely similar to how Rust works). But poetry is not the only build system that exists. Poetry is the new de-facto default so to speak (for most cases, at least). But if you are build Rust-Python binding for example, you wouldn't use poetry as backend, you would use maturin as backend, but it would still use pyproject.toml. The idea is that setup.py is deprecated officially based on PEP518, and pyproject.toml can provide deterministic builds based on many different build backends of your choosing (poetry being, as mentioned the de-facto standard in most cases).

It does look like I need to include packaging in the setup.py install_requires list --- it's currently only in requirements.txt (which is usually what I use to build + install jigsawpy).

Not 100% sure what you mean by this, but, give me a few days and I'll provide more guidance on how to test poetry.

What I can tell you is the following, look at almost every Python package that is up to date and almost every package has switched to poetry (where applicable). Poetry still uses pip in the backend, but poetry is deterministic, while setup.py is considered a lot more ad-hoc. Unfortunately, setup.py based builds do not play well with poetry based builds, and that's why I've been using my own forks on my builds.

Give me some time and I can put together more examples on how to test, in the meantime, I would suggest you look into poetry and test the branches I've put together, particularly with poetry build, e.g.

poetry shell
poetry build
pip install .

One thing that poetry does a lot better than setup.py is build the wheels. Poetry will include the binaries.
I was skeptical about poetry at first too, but after I tested it, I noticed why the change. I think you should test it to understand better what it does. It does not replace pip, rather it compliments pip and fills some holes that pip doesn't cover. It's hard to explain thoroughly, but it follows the same model as Rust, and the change does make sense after one tests it.

In my case I had to make this fork and implement the change because my own build are poetry based and jigsapy/inpoly were not compiling correctly without a proper pyproject.toml. Now every works seamlessly, and everything gets compiled in the background when I do pip install .

@jreniel
Copy link
Author

jreniel commented Dec 28, 2023

Also worth mentioning, this doesn't force you to have poetry installed or directly use poetry. I think it works just the same if you clone it and do pip install . with the difference that pip will now use poetry as backend and will know how to resolve the versions correctly including the compilation with gcc and wheel building.

@jreniel
Copy link
Author

jreniel commented Dec 28, 2023

[build-system]
requires = [
"setuptools>=42",
"wheel",
"ninja",
"cmake>=3.12",
]
build-backend = "setuptools.build_meta"

The snippet above is an example of a project that uses setup.py and pyproject.toml ... I've learned more about this topic since I opened this PR, and one thing that perhaps I should (self) clarify is that, it's not that setup.py is getting deprecated, it's more that pip is now more pyproject.toml driven, and setup.py or build.py play a supporting, rather than a primary role.
So in short, what we mainly need is a pyproject.toml, and it doesn't have to be poetry driven, but IMO using poetry as build backend does have advantages, but it's not as essential. What's more or less essential is the inclusion of a pyproject.toml to conform to PEP518

@dengwirda
Copy link
Owner

Thanks for your thoughts on this @jreniel, I appreciate it!
Reading a little, and checking what eg. numpy, scipy, pytorch, etc do I agree with your last --- adding a minimal pyproject.toml with a setuptools backend seems like it may be a good option for jigsawpy at this stage.
There are indeed nice things written about poetry, but forcing this on folks is something I'd prefer not to do. It seems there are a number of backends fighting it out presently, so unfortunately not much opportunity for standardisation in that space yet.
If you're keen to modify this PR to take a more minimal approach I'll be happy to merge it. What I'd like to do is maintain as much backwards compatibility as possible, so that someone on a system with a very old python is still able to build via the usual python setup.py build_external install, while pip install -e . or equiv. is a replacement going forward.

@jreniel
Copy link
Author

jreniel commented Jan 10, 2024

Hi @dengwirda,
Sorry I haven't had time yet to look into this. I've stumbled upon packages that specify more than one tooling option on pyproject.toml, that is, both a setuptools based one and a poetry based one. Like I mentioned before, adding a poetry based backend I think may be transparent to the end user, but I'd have to test. What I know for sure is that officially, the build system is migrating to being pyproject.toml-based and that's why you see all the major packages now including a pyproject.toml.

I'll try to make room to modify this into a setuptools based and potentially with a poetry entry as well, so that it can play well with as many users as possible.

But if you happen to have someone that beats me to the punch on this, feel free to close this and merge the other one! I won't feel left out or offended.
Thanks again for considering this PR.

@dengwirda
Copy link
Owner

Thanks @jreniel no worries at all --- I'll have a look at supporting a minimal *.toml approach.

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.

2 participants