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 poetry support #4754

Closed
wants to merge 81 commits into from
Closed

add poetry support #4754

wants to merge 81 commits into from

Conversation

timmysilv
Copy link
Contributor

@timmysilv timmysilv commented Oct 30, 2023

Context:
Poetry helps manage dependencies in a stable way for reproducibility. PennyLane can benefit from this!

Description of the Change:
Update pyproject.toml to encapsulate the various requirements of PL grouped by task (to match the various existing requirement files). These are the details I think reviewers should consider:

  • TensorFlow doesn't support 3.12, but we do, so I had to hack it in with pip all over
    • All docs build use the new, (mostly) auto-generated version of doc/requirements.txt instead of poetry install, but the file is more complete and stable than before
    • The make lock target that I added hacks in a hard-coded tensorflow version using shell commands
    • TF 2.15.0 requires ml-dtypes<0.3, but Python 3.12 requires ml-dtypes>=0.3, so that's hacked in as well
  • Other than the above stuff, our GitHub workflow files are much neater:) anything installed with pip (catalyst, lightning, tf) run the risk of downgrading packages. See appendix below for details on this risk
  • The aforementioned make lock target uses poetry to generate all files, then it changes doc/requirements.txt manually (very bad!) because docs fail with torch>=2, and it needs those TF support changes. I wouldn't read much into the requirements*.txt files other than how they're made (aka what this target calls)
  • I put all relevant Poetry details into the installation doc, lmk what you think
  • I'm not sure if I need everything in the [tool.poetry] section of pyproject.yaml, or if it should inherit from some other section later? but this worked.
  • I don't want to delete anything from setup.py yet, but that should come soon after this for PEP517/518 support.
  • I set as many minimum bounds as possible, but I can't guarantee the correctness of all of them. I often used some_pkg = "^x.y.z", which is the same as >=x.y.z,<x+1. This feels safe for explicit dependencies, but lmk what you think
  • cvxopt couldn't find wheels for 3.12, so I listed their links manually
  • sphinx is just set to ~3.5.0, but we want 4.2.0 for python 3.10+. Unfortunately, if you have 2 versions of a dependency, it will block you from exporting it to a requirements.txt file, so I removed the 4.2.0 option (because we build docs with 3.9 for now)
  • Poetry dependency groups are required by default, and we don't want that, so I mark them all as optional at the top

Benefits:
We can offer a more reliable and standard environment initialization in this way

Possible Drawbacks:
More complicated, might involve more maintenance. Ideally it does not, but this will depend on how we introduce and work with it. Anything installed with pip can compromise the benefits. TensorFlow, lightning, catalyst.

Appendix A - Downgrading things installed with Poetry

Today, we have jax==0.4.23 (latest) in our lockfile, but catalyst requires an earlier version (0.4.14). As you can see, it succeeds in downgrading jax, jaxlib and ml-dtypes because they were originally installed with poetry, and the new versions also fit in the range specified by pyproject.toml. Not the best because it's different from the lockfile, but not the worst because it's consistent with the toml file.

Appendix B - Downgrading things installed from requirements.txt

Because of this TF nonsense, as well as the custom sphinx action we run that expects requirements.txt, I am using that file to install all dependencies when building docs. That said, I struck a deal - the file is mostly auto-generated by poetry. However, the installation is ultimately done by pip, and that leads to an issue. In this case, the requirements file specified ml-dtypes==0.3.2 (mentioned above, multiple reasons why this had to be) and TF wanted to downgrade it. Pip raised an error, because it thought that I really, really needed 0.3.2 in all cases, even this docs CI run using python 3.9 (only 3.12 needs this). So, the make target replaces the version in the requirements file automatically.

[sc-49009]

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (35af47f) 99.69% compared to head (eeefa5c) 99.69%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4754   +/-   ##
=======================================
  Coverage   99.69%   99.69%           
=======================================
  Files         394      394           
  Lines       36073    36120   +47     
=======================================
+ Hits        35962    36009   +47     
  Misses        111      111           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but I don't want to remove it (or any existing files) in this PR to not disturb people who might depend on it. lord knows who would other than CI (it's aptly named!) but just in case. if people are ok with deleting it, I will do that with pleasure.

Comment on lines 115 to 117
pip install --upgrade pip
pip install --upgrade poetry wheel
poetry config virtualenvs.create false
Copy link
Contributor

Choose a reason for hiding this comment

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

I would highly recommend usage, of an action that can setup poetry. It's being used in QML as well See Here

Link to the action itself: https://github.com/XanaduAI/cloud-actions/tree/main/install-python-poetry

The benefit of using the action is that it installs poetry into a separate env to ensure poetry never attempts to manage it's own dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

already a cloud-action 😍 i'll give it a shot


[tool.poetry]
name = "pennylane"
version = "0.35.0-dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to maintain the version of pennylane in two spots? (Pyproject + pennylane/_version.py)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not, but that was my short-term plan. can we have one read from the other?

Copy link
Contributor

@rashidnhm rashidnhm Jan 31, 2024

Choose a reason for hiding this comment

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

So apparently the Python answer is a package should not need to know it's own version within it's own code. As there shouldn't be logic within the package itself that alters behavior using the version.

Now if someone decides to use the package and has logic based on version, then they can get the package version using importlib.metadata.version("{package}").

This would mean pyproject.toml being the ultimate source of truth.

Now if for backwards-compatibility reason, you want to continue to provide package.__version__, then yeah, you can have it read the pyproject.

I've actually done this for the Architecture team, see this: Link

You add toml as a dependency, then return the packages own version from importlib.metadata. If someone is using package in editable mode for development, then read pyproject.

EDIT: Nvm, it's still perfectly acceptable to have a version.py file .. but rather it is poetry that does not support dynamically reading the version.py file (without plugins at least).. but you can still use the link above if you'd like. Poetry Issue Link

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be out of scope ... but can't we just drop the usage of this actions? Then use RTD's fail-on-warning configuration instead?

https://docs.readthedocs.io/en/stable/config-file/v2.html#sphinx-fail-on-warning

This way the RTD build will fail if sphinx raises a warning, which to my understanding is what this action was aimed at doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually know enough about this action, but I'm very open to the idea. say.. it wouldn't mind if we were using sphinx 3, would it?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't know about fail on warning for RTDs! That was the main reason we had a separate action for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, on the PR description I see that this action is one of the reason to keep requirements-*.txt file, RTD supports poetry and pyproject just fine. Would be a cleaner solution in my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well.. we can't quite use poetry alone for RTD because of tensorflow and torch issues, but that's a whole other thing : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #5136 to make it happen

@@ -66,6 +66,84 @@ importing PennyLane in Python.
requires ``pip install -e .`` to be re-run in the plugin repository
for the changes to take effect.

Poetry
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could we update the installation section above to recommend use of poetry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of us explicitly endorsing Poetry in this way. I think this needs to be reworded as "supports poetry" rather than explicitly want people to use this. This isn't something we'll be adding to Lightning or Catalyst, so making it an optional utility instead is what I'd prefer. Can we make this update?

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Just a quick comment. I'm still not 100% convinced poetry is better than us being fully PEP compliant, and want to make sure that is the standard first, rather than explicitly depending on Poetry. This will also influence work on Lightning and later Catalyst.

Happy to be involved in discussion around this, but I'm not sure about merging this in just yet.

@@ -66,6 +66,84 @@ importing PennyLane in Python.
requires ``pip install -e .`` to be re-run in the plugin repository
for the changes to take effect.

Poetry
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of us explicitly endorsing Poetry in this way. I think this needs to be reworded as "supports poetry" rather than explicitly want people to use this. This isn't something we'll be adding to Lightning or Catalyst, so making it an optional utility instead is what I'd prefer. Can we make this update?

@timmysilv
Copy link
Contributor Author

@mlxd can you elaborate on these concerns re: lightning and catalyst? I feel like this PR should be able to remain as a standalone feature. Everything else will continue to use and be compatible with pip and the current tooling. The only thing this is really doing is using poetry to generate a lockfile, then to install the dependencies from that lockfile before running CI.

I don't feel strongly that we should recommend it to users for installation (hence the only-recent addition), and am fine to remove it. But I will say, it's a much nicer experience and if you like poetry, you might like this too. It's deep in a developer doc, but I'm good either way.

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Just a follow-on: I think as is this redefines how we are spec'ing PennyLane and adds a departure from the pip-first approach we have defaulted to elsewhere in the ecosystem we may want to rethink this.

If poetry can just be some additional helper on top of an already PEP 517/518/621 compliant package, and just does some heavy lifting in the CI, all good. If we still need to redefine parts of the project files and add all this extra machinery, I think we may want to step back from poetry for now as it may not be fit for purpose.

Let me know if making the changes isn't possible or easily done, and if not we can chat more about this with @trbromley and @Alex-Preciado as needed.

Comment on lines +1 to +30
contourpy==1.2.0 ; python_version >= "3.9" and python_full_version < "4.0.0"
cvxopt @ https://files.pythonhosted.org/packages/10/dc/1c21715e1267ca29f562e4450426d1ff8a7ffcc3e670100cec332a105b95/cvxopt-1.3.2-cp312-cp312-macosx_10_9_x86_64.whl ; sys_platform == "darwin" and platform_machine == "x86_64" and python_version == "3.12"
cvxopt @ https://files.pythonhosted.org/packages/9f/ad/edce467c24529c536fc9de787546a1c8eca293009383a872b6f638d22eae/cvxopt-1.3.2-cp312-cp312-win_amd64.whl ; python_version == "3.12" and sys_platform == "windows"
cvxopt @ https://files.pythonhosted.org/packages/c7/17/ee82c745c5bda340a4dd812652c42fb71efd45f663554a10c3ec45f230df/cvxopt-1.3.2-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl ; python_version == "3.12" and sys_platform == "linux"
cvxopt @ https://files.pythonhosted.org/packages/cd/c8/a04048143d0329ccd36403951746c1a6b5f1fc56c479e5a0a77efb2064b2/cvxopt-1.3.2-cp312-cp312-macosx_11_0_arm64.whl ; sys_platform == "darwin" and platform_machine == "arm64" and python_version == "3.12"
cvxopt==1.3.2 ; python_full_version >= "3.9.0" and python_version < "3.12"
cvxpy==1.4.1 ; python_full_version >= "3.9.0" and python_full_version < "4.0.0"
cycler==0.12.1 ; python_version >= "3.9" and python_full_version < "4.0.0"
ecos==2.0.12 ; python_full_version >= "3.9.0" and python_full_version < "4.0.0"
fonttools==4.46.0 ; python_version >= "3.9" and python_full_version < "4.0.0"
importlib-resources==6.1.1 ; python_version >= "3.9" and python_version < "3.10"
kiwisolver==1.4.5 ; python_version >= "3.9" and python_full_version < "4.0.0"
matplotlib==3.8.0 ; python_version >= "3.9" and python_full_version < "4.0.0"
numpy==1.26.2 ; python_version >= "3.9" and python_full_version < "4.0.0"
osqp==0.6.3 ; python_full_version >= "3.9.0" and python_full_version < "4.0.0"
packaging==23.2 ; python_version >= "3.9" and python_full_version < "4.0.0"
pillow==10.1.0 ; python_version >= "3.9" and python_full_version < "4.0.0"
pybind11==2.11.1 ; python_full_version >= "3.9.0" and python_full_version < "4.0.0"
pyparsing==3.1.1 ; python_version >= "3.9" and python_full_version < "4.0.0"
python-dateutil==2.8.2 ; python_version >= "3.9" and python_full_version < "4.0.0"
qdldl==0.1.7.post0 ; python_full_version >= "3.9.0" and python_full_version < "4.0.0"
scipy==1.11.4 ; python_version >= "3.9" and python_full_version < "4.0.0"
scs==3.2.4.post1 ; python_full_version >= "3.9.0" and python_full_version < "4.0.0"
setuptools-scm==8.0.4 ; python_version >= "3.9" and python_full_version < "4.0.0"
setuptools==69.0.3 ; python_version >= "3.9" and python_full_version < "4.0.0"
six==1.16.0 ; python_version >= "3.9" and python_full_version < "4.0.0"
tomli==2.0.1 ; python_version >= "3.9" and python_version < "3.11"
typing-extensions==4.8.0 ; python_version >= "3.9" and python_full_version < "4.0.0"
zipp==3.17.0 ; python_version >= "3.9" and python_version < "3.10"
Copy link
Member

Choose a reason for hiding this comment

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

This is pinning packages where we didn't before. Is that necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depends what you mean by necessary. Some notes on that:

  • as discussed in another comment, this file can be deleted altogether if we don't think people depend on it. CI doesn't use it anymore (in this PR), I just re-generated it to not delete a file that users might have counted on
  • the pinning is just how poetry export does it, because that's how Poetry itself behaves. You give it acceptable version ranges, it gives you one exact set of compatible versions of each dependency

I don't want to maintain both poetry and pip-style files manually - if we decide that we still have to maintain these manually for some reason, then I have some hesitations moving forward

pyproject.toml Outdated
Comment on lines 90 to 150
[tool.poetry.group.ci.dependencies]
cvxpy = "^1.4.1"
cvxopt = [
{ python = "<3.12", version = "^1.3.2" },
{ python = "3.12", platform = "linux", url = "https://files.pythonhosted.org/packages/c7/17/ee82c745c5bda340a4dd812652c42fb71efd45f663554a10c3ec45f230df/cvxopt-1.3.2-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl" },
{ python = "3.12", markers = "sys_platform == 'darwin' and platform_machine == 'x86_64'", url = "https://files.pythonhosted.org/packages/10/dc/1c21715e1267ca29f562e4450426d1ff8a7ffcc3e670100cec332a105b95/cvxopt-1.3.2-cp312-cp312-macosx_10_9_x86_64.whl" },
{ python = "3.12", markers = "sys_platform == 'darwin' and platform_machine == 'arm64'", url = "https://files.pythonhosted.org/packages/cd/c8/a04048143d0329ccd36403951746c1a6b5f1fc56c479e5a0a77efb2064b2/cvxopt-1.3.2-cp312-cp312-macosx_11_0_arm64.whl" },
{ python = "3.12", platform = "windows", url = "https://files.pythonhosted.org/packages/9f/ad/edce467c24529c536fc9de787546a1c8eca293009383a872b6f638d22eae/cvxopt-1.3.2-cp312-cp312-win_amd64.whl" },
]
matplotlib = "^3.5.0"

[tool.poetry.group.doc.dependencies]
mistune = "0.8.4"
m2r2 = "^0.3.2"
pygments-github-lexers = "^0.0.5"
semantic-version = "2.10"
docutils = "0.16"
sphinx = "~3.5.0" # must only provide one to export, but should be 4.2.0 for python >= 3.10
sphinx-automodapi = "0.13"
sphinx-copybutton = "^0.5.2"
sphinxcontrib-bibtex = "2.4.2"
sphinxcontrib-applehelp = "1.0.4"
sphinxcontrib-devhelp = "1.0.2"
sphinxcontrib-qthelp = "1.0.3"
sphinxcontrib-htmlhelp = "2.0.1"
sphinxcontrib-serializinghtml = "1.1.5"
tensornetwork = "0.3"
jinja2 = "3.0.3"
# we do not pin the sphinx theme, to allow the
# navbar and header data to be updated at the source
pennylane-sphinx-theme = "*"
sphinxext-opengraph = "0.6.3" # Note: latest version 0.9.0 requires Sphinx >=4.0
matplotlib = "*"

[tool.poetry.group.jax.dependencies]
jax = "^0.4.14"
jaxlib = "^0.4.14"

[tool.poetry.group.torch.dependencies]
torch = [
{ version = "2.1.0", source = "PyPI", platform = "darwin" },
{ version = "2.1.0+cpu", source = "pytorch-cpu", platform = "!=darwin" },
]

[tool.poetry.group.external.dependencies]
pyzx = "^0.7.1"
matplotlib = "*"
stim = "*"

[tool.poetry.group.qcut.dependencies]
kahypar = "^1.1.7"
opt_einsum = "*"

[tool.poetry.group.qchem.dependencies]
openfermionpyscf = "*"
basis-set-exchange = "*"

[tool.poetry.group.data.dependencies]
h5py = "*"
aiohttp = "*"
fsspec = "*"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a standard (aka non-poetry) way to handle this information in the pyproject.toml file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what I see, no. PEP517 and more importantly here, PEP518 are not very clear on the matter. It seems that 3P orgs can define their own 517-compliant build backend, then provide it, and that's exactly what poetry does.

Comment on lines +15 to +17
[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be converting the PEP-517 complaint build system here to be using Poetry by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned on your other comment, I'm not sure if it's the wrong thing to do. Poetry claims it's standard, but I'll admit that I don't know what other 517-compliant packages look like

"default.mixed" = "pennylane.devices.default_mixed:DefaultMixed"
"null.qubit" = "pennylane.devices.null_qubit:NullQubit"
"default.qutrit" = "pennylane.devices.default_qutrit:DefaultQutrit"

Copy link
Member

Choose a reason for hiding this comment

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

Can all poetry-specific sections here and below instead conform with the standardized PEP (e.g. 517/621/etc) ways of specifying these sections? I'd rather we didn't have poetry-specifics in something that is used to send the ecosystem out into the wild, but is only used to help manage the CI and inter-related deps therein.

Adding anything poetry specific here that becomes ingrained cannot then be used with pip/conda/etc, as it isn't standardized by Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to change it to use non-poetry stuff locally, but poetry can only interpret its own headers. I imagine that they align for the most part, but if we intend on adding non-poetry configuration (eg. [project] instead of [tool.poetry], and [project.entry-points."pennylane.plugins"] instead of [tool.poetry.plugins."pennylane.plugins"]), there will just have to be a lot of duplication. Those cases are just s/tool.poetry/project, but that won't always be the case.

The README for their build-system makes me think that we will in fact require people to have poetry-core installed in order to pip install pennylane, but that should only be from source.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @timmysilv
I think we should discuss this more next week with @trbromley and @Alex-Preciado and see what makes the most sense here.

@timmysilv
Copy link
Contributor Author

doing #5158 instead

@timmysilv timmysilv closed this Feb 7, 2024
@trbromley trbromley deleted the use-poetry branch February 8, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:run-full-test-suite Run the full test-suite on the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants