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

move to poetry for dependency management; consolidate more settings into pyproject.toml #2187

Merged
merged 29 commits into from
Jan 17, 2023

Conversation

jclerman
Copy link
Contributor

@jclerman jclerman commented Dec 29, 2022

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

  • Fixes to tox.ini are still needed. DONE
  • Updates to pyproject.toml are needed, to add more dependency groups, which should then be referenced in tox.ini and other build/linting configs that have steps requiring development-dependencies. DONE
  • Remove references to requirements*txt files from
    • Taskfile.yaml DONE
    • workflows/validate.yaml DONE
  • Remove requirements*txt files once they are no longer referenced anywhere. DONE

Other details

  • With this change, versions of package dependencies are now explicit.
  • This change eliminates the use of static requirements*txt files from Dockerfile.devcontainer

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Checked that all tests and type checking passes.
  • For changes that have a potential impact on users of this project:
    • Updated relevant documentation to avoid inaccuracies.
    • Considered adding additional documentation.
    • Considered updating our changelog (CHANGELOG.md). (Elected not to update CHANGELOG.md since this PR does not have user-facing changes)
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

 - also consolidating other settings to pyproject.toml when possible
@aucampia
Copy link
Member

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

pyproject.toml Outdated Show resolved Hide resolved
@aucampia
Copy link
Member

aucampia commented Dec 30, 2022

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

 - both envs fail when run - not clear that the failures are new since they seem unrelated to my changes
@jclerman
Copy link
Contributor Author

@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.

@aucampia
Copy link
Member

@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
Copy link
Contributor Author

jclerman commented Dec 30, 2022

@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.

tox.ini Show resolved Hide resolved
…est poetry-core.

 - poetry-core is at 1.4.0; require that to avoid risk of encountering old bugs
 - "flake8" was causing problems with pip during validate jobs, in which pip
   tried to do this:

    py37: install_package_deps> python -I -m pip install black==22.12.0 'flake8>=4.0.1; or extra == "flake8"' 'flakeheaven<4.0.0,>=3.2.1; or extra == "flake8"' 'importlib_metadata<5.0.0,>=4.2.0; python_version >= "3.7" and python_version < "3.8"' 'isodate<0.7.0,>=0.6.1' 'isort<6.0.0,>=5.10.0' mypy==0.991 'networkx<3.0.0,>=2.6.2; (python_full_version > "3.7.0" and python_version < "3.8") and extra == "networkx"' 'networkx<3.0.0,>=2.8.8; (python_version >= "3.8" and python_version < "4.0") and extra == "networkx"' 'pep8-naming<0.14.0,>=0.13.2; or extra == "flake8"' 'pyparsing<4.0.0,>=3.0.9' 'pytest-cov<5.0.0,>=4.0.0' 'pytest<8.0.0,>=7.1.3'

    and complained about:

    pip._vendor.packaging.markers.InvalidMarker: Invalid marker: 'or extra == "flake8"', parse error at 'or extra'
@jclerman
Copy link
Contributor Author

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

@aucampia
Copy link
Member

aucampia commented Jan 2, 2023

@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.

Dockerfile.devcontainer Outdated Show resolved Hide resolved
@aucampia
Copy link
Member

aucampia commented Jan 2, 2023

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

@aucampia
Copy link
Member

aucampia commented Jan 2, 2023

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
Copy link
Contributor Author

jclerman commented Jan 2, 2023

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

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

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?

@aucampia
Copy link
Member

aucampia commented Jan 2, 2023

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
Copy link
Member

aucampia commented Jan 3, 2023

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
Copy link
Member

aucampia commented Jan 3, 2023

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
Copy link
Member

aucampia commented Jan 3, 2023

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 and others added 6 commits January 16, 2023 00:04
…mmands inside the devcontainer with docker volumes from the host
This should avoid interference between different containers and the host
environment.
@jclerman jclerman marked this pull request as ready for review January 16, 2023 03:54
@aucampia
Copy link
Member

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
Copy link
Member

aucampia commented Jan 16, 2023

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
Copy link
Member

CC: tagging you in case you have time for a review @cclauss @eggplants @mwatts15

@mwatts15
Copy link
Contributor

mwatts15 commented Jan 16, 2023 via email

@aucampia
Copy link
Member

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.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

A few possible upgrades

.github/workflows/docker-images.yaml Outdated Show resolved Hide resolved
.github/workflows/docker-images.yaml Outdated Show resolved Hide resolved
@aucampia
Copy link
Member

Thanks for the review @cclauss

@aucampia
Copy link
Member

@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.

Copy link
Member

@aucampia aucampia left a comment

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
Copy link
Contributor Author

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.

pyproject.toml Outdated Show resolved Hide resolved
@mwatts15
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

jclerman commented Jan 17, 2023

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
Copy link
Contributor

mwatts15 commented Jan 17, 2023 via email

@mwatts15
Copy link
Contributor

mwatts15 commented Jan 17, 2023

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
Copy link
Member

aucampia commented Jan 17, 2023

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.

change `Black` -> `isort` in isort config.
@aucampia aucampia requested a review from a team January 17, 2023 19:35
@aucampia
Copy link
Member

Planning to merge a bit later tonight (CET).

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.

6 participants