move to poetry for dependency management; consolidate more settings into pyproject.toml by jclerman · Pull Request #2187 · RDFLib/rdflib (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation75 Commits29 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

jclerman

Summary

Goals:

  1. move from setup.py & setuptools to poetry, for dependency and project management.
  2. move other settings, from setup.cfg, into pyproject.toml.
  3. Consolidate all dependencies into one file, pyproject.toml, and remove the need for all requirements*txt files.

Includes moving the package version into pyproject.toml, and in __init__.py, reading the version from pyproject.toml via use of importlib.metadata.

TODO

Other details

Checklist

@jclerman

@aucampia

@jclerman thanks for this, would be very happy to get to poetry as that makes things a lot easier.

aucampia

aucampia

aucampia

@aucampia

@jclerman let me know if you are okay with me commiting to your branch.

@jclerman

@jclerman

@jclerman let me know if you are okay with me commiting to your branch.

@aucampia that should be fine. I wanted to make a few additional changes to demonstrate how we can get tox to use pyproject.toml-specified dependencies, without the use of extras which don't quite make sense since they are intended only for development use, not for end-users. I have that working now (I think) - though I am seeing issues with both the flake8 and docs builds, at least locally. Not clear to me why either of the failures are occurring, or whether they are even new.

@jclerman

@aucampia

@jclerman I won't make changes to your branch, I will make a separate branch with any changes that I think may be relevant and link to it here, and I will merge your branch into it when there is changes.

@jclerman

@jclerman

@aucampia, for the time being, I have updated pyproject.toml to preserve all of the "extras" groups as they were previously configured. I think we agree that it'd be nice to remove the "extras" that aren't intended for end-users, replacing them with poetry dependency-groups (as was my initial plan). However, there are a lot of moving parts here, and it probably make sense to stay focused on the move to poetry, and to tackle the migration away from extra "extras" as a follow-up PR.

Unfortunately, tox (and probably other tools) does not (yet?) support built-in access to poetry dependency-groups. The way I have accessed them in other projects is to add lines like this to a given tox-environment's config clause:

allowlist_externals = poetry
commands_pre =
    poetry export --only XYZ --output {temp_dir}/{envname}-requirements-XYZ-only.txt
    pip install -r {temp_dir}/{envname}-requirements-XYZ-only.txt

replacing "XYZ" with the name of the poetry dep-group I want to use for that tox-env.

@jclerman

jclerman

tox.ini Show resolved Hide resolved

@jclerman

…est poetry-core.

@jclerman

I think just one issue remains to address before we can deprecate setup.py: The following clause in that file provides logic for conditional installation of the examples package. That kind of conditional installation is not supported through poetry, as far as I can tell. I'm also not sure if the condition is ever met, since I couldn't find the READTHEDOCS environment-variable set in any of the build-configs - but maybe it's set elsewhere?

This is the clause:

if os.environ.get("READTHEDOCS", None):
    # if building docs for RTD
    # install examples, to get docstrings
    packages.append("examples")

I think the simplest thing here would be to move examples down into the rdflib distribution and make it a sub-package (which is always included in the build), and to update docs/apidocs/examples.rst accordingly. If there is a simpler solution though, that'd be great. @aucampia

@jclerman

…stead of requirements.txt files

@jclerman

…ependencies

@jclerman

@aucampia

@jclerman thanks for all the work here, I will look at what we can do about the examples, I also think it may not be needed.

aucampia

@jclerman

@aucampia

I think we should just drop flakeheaven, it is sad, but they are too far behind on flake8.

@aucampia

We have to use sphinx>=5, and if I set that constraint flake8 4.x becomes an issue. I will see what I can do to run flake8 only on the diffs, but will do that later, for now I would rather go without flake8. I will look at it further tomorrow. My branch is https://github.com/aucampia/rdflib/tree/switch-to-poetry

@jclerman

@aucampia

Are there tests (or can we make one) that show what breaks when we use sphinx 4.x?

Sphinx 4 creates warnings with python 3.7 relating to type hints and these are set to result in errors. We can maybe disable that but then we will miss other real problems, but maybe just disabling it on python 3.7 makes sense.

Would it be helpful if I were to start manually merging your changes into this (https://github.com/jclerman/rdflib/tree/switch-to-poetry) branch?

I wouldn't yet. Let me figure out things first, I will clean it up as much as I can and make sure it only contains things that really should be in the same PR. I should be done tomorrow.

@aucampia

Some thoughts ...

The main benefit we want from poetry is reproducible dev environments, which we can get from poetry's lockfile. However, for this to provide maximal value we really need to take this into account for testing also. I'm looking at options now.

Further, we should be as liberal with version restrictions that end up in the wheel, we should not really place upper limits unless we know later versions break, and the lower limits should be as low as possible.

As for the importlib-metadata and flakeheaven situation, I'm trying to resolve it without removing flakeheaven.

@aucampia

The main benefit we want from poetry is reproducible dev environments, which we can get from poetry's lockfile. However, for this to provide maximal value we really need to take this into account for testing also. I'm looking at options now.

Somewhat burying the lede here, to just clarify, we really need to take this into account for tox. So tox should use the versions from poetry.lock. At least I think so. Maybe we can look at it later though.

@aucampia

The main benefit we want from poetry is reproducible dev environments, which we can get from poetry's lockfile. However, for this to provide maximal value we really need to take this into account for testing also. I'm looking at options now.

Somewhat burying the lede here, to just clarify, we really need to take this into account for tox. So tox should use the versions from poetry.lock. At least I think so. Maybe we can look at it later though.

yeah I will figure it out in a later PR.

@edmondchuc @aucampia

@edmondchuc @aucampia

@edmondchuc @aucampia

…o install poetry with all extras flag.

@edmondchuc @aucampia

…mmands inside the devcontainer with docker volumes from the host

@aucampia

This should avoid interference between different containers and the host environment.

@jclerman

@jclerman jclerman marked this pull request as ready for review

January 16, 2023 03:54

@aucampia

I will do a final review tonight and maybe wait a day or so for more reviews.

Thanks for all the effort and patience.

CC: tagging in case anyone has time to review @rchateauneu @edmondchuc @RDFLib/core-reviewers @mwatts15

@aucampia

I will fix the build error tonight - super weird that it worked fine in #2192 and broke in this PR while the two branches look identical to me 🤔

@aucampia

@mwatts15

@aucampia

Can't likely do a full review, but I can try out basic project setup stuff and verify, if that helps.

@mwatts15 That will be appreciated if you have time.

cclauss

Choose a reason for hiding this comment

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

A few possible upgrades

@aucampia

@aucampia

@aucampia

@jclerman I bumped the versions now in:

In a subsequent PR we can create a devtools/requirements-gha.txt with tox poetry, and then have dependabot manage it and use it to install tox and poetry in the github actions pipeline.

aucampia

Choose a reason for hiding this comment

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

Thanks for the effort and patience, I think poetry makes rdflib a lot easier to develop for newcomers, and I do prefer it to manually managing my venv.

There are some more things we can do in subsequent PRs but I think this is good to merge.

@jclerman

Thanks so much @aucampia for your huge effort refining my initial PR and helping get this over the finish line. I agree that poetry will make development/contributions much easier for newcomers (:wave:), and looking forward to making contributions to the library itself once this is in place.

mwatts15

@mwatts15

Possibly a Poetry noob issue, but after I do a poetry install, I can't run poetry anymore because poetry install uninstalls html5lib (I take it that's because poetry uninstalls extras you don't specify and html is an extra...). This is surprising to me -- I'd think I could poetry install again and basically poetry would maybe check what's installed is up to date (compare against poetry.lock and pyproject.toml, etc.).

@mwatts15

I made a pretty shallow review. I really like how so much of the project configuration is laid out in one file so I don't have to go hunting. Good stuff!

@jclerman

Possibly a Poetry noob issue, but after I do a poetry install, I can't run poetry anymore because poetry install uninstalls html5lib (I take it that's because poetry uninstalls extras you don't specify and html is an extra...). This is surprising to me -- I'd think I could poetry install again and basically poetry would maybe check what's installed is up to date (compare against poetry.lock and pyproject.toml, etc.).

Thanks @mwatts15! Yes, poetry install with no flags for extras does seem to remove any extras previously installed. Not sure what you mean though by not being able to run poetry anymore?

*The removal of extras is buried (IMO) in poetry's documentation; check https://python-poetry.org/docs/cli#install, which notes:

Extras are not sensitive to --sync. Any extras not specified will always be removed.

@mwatts15

sorry, should have posted the stack trace. poetry seems to have an html5lib dependency. it tries to import the library for all the commands I tried.

…

On Mon, Jan 16, 2023, 21:06 Jeffrey C. Lerman ***@***.***> wrote: Possibly a Poetry noob issue, but after I do a poetry install, I can't run poetry anymore because poetry install uninstalls html5lib (I take it that's because poetry uninstalls extras you don't specify and html is an extra...). This is surprising to me -- I'd think I could poetry install again and basically poetry would maybe check what's installed is up to date (compare against poetry.lock and pyproject.toml, etc.). Thanks @mwatts15 <https://github.com/mwatts15>! Yes, poetry install with no flags for extras does seem to remove any extras previously installed. Not sure what you mean though by not being able to run poetry anymore though? — Reply to this email directly, view it on GitHub <#2187 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALLFSDT43Y7A7FEVOKCC3LWSYEE3ANCNFSM6AAAAAATLZ6IUA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

@mwatts15

sorry, should have posted the stack trace. poetry seems to have an html5lib dependency. it tries to import the library for all the commands I tried.

Here's the error poetry gives me. It doesn't seem like html5lib should be needed really and especially doesn't seem like poetry should let you uninstall its own dependencies. I just did pip install poetry in a virtualenv (https://python-poetry.org/docs/#installing-manually), so I don't think I did anything weird.

% poetry --verbose                                                                                                                                                                                                                                                       1

  ModuleNotFoundError

  No module named 'html5lib'

  at env/lib/python3.10/site-packages/poetry/repositories/link_sources/html.py:22 in <module>
       18│ 
       19│ 
       20│ with warnings.catch_warnings():
       21│     warnings.simplefilter("ignore")
    →  22│     import html5lib
       23│ 
       24│ 
       25│ class HTMLPage(LinkSource):
       26│     def __init__(self, url: str, content: str) -> None:

The following error occurred when trying to handle this error:


  ModuleNotFoundError

  No module named 'html5lib'

  at env/lib/python3.10/site-packages/poetry/repositories/link_sources/html.py:22 in <module>
       18│ 
       19│ 
       20│ with warnings.catch_warnings():
       21│     warnings.simplefilter("ignore")
    →  22│     import html5lib
       23│ 
       24│ 
       25│ class HTMLPage(LinkSource):
       26│     def __init__(self, url: str, content: str) -> None:

@aucampia

I just did pip install poetry in a virtualenv (https://python-poetry.org/docs/#installing-manually), so I don't think I did anything weird.

If you do poetry install with VIRTUAL_ENV set then then poetry will use that virtualenv instead of the project specific virtualenv, and if VIRTUAL_ENV is set to the poetry virtual env then it will mess with poetry's dependencies.

I usually use pipx for poetry, as that then manages my venv for me, and then poetry works quite well. If you manually made a poetry venv just be sure to not have it active when running poetry install, rather use the full path to ${VIRTUAL_ENV}/bin/poetry or add ${VIRTUAL_ENV}/bin to PATH - just as long as VIRTUAL_ENV is not your poetry venv when poetry install runs.

@aucampia

change Black -> isort in isort config.

@aucampia

Planning to merge a bit later tonight (CET).