-
Notifications
You must be signed in to change notification settings - Fork 558
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
Conversation
- also consolidating other settings to pyproject.toml when possible
@jclerman thanks for this, would be very happy to get to poetry as that makes things a lot easier. |
@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
@aucampia that should be fine. I wanted to make a few additional changes to demonstrate how we can get tox to use |
@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. |
@aucampia, for the time being, I have updated 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:
replacing "XYZ" with the name of the poetry dep-group I want to use for that tox-env. |
…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'
I think just one issue remains to address before we can deprecate This is the clause:
I think the simplest thing here would be to move |
…stead of requirements.txt files
…ependencies - exception: docker:prepare task is not yet updated
@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. |
I think we should just drop flakeheaven, it is sad, but they are too far behind on flake8. |
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? |
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.
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. |
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. |
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. |
This reverts commit a4da002.
…o install poetry with all extras flag.
…mmands inside the devcontainer with docker volumes from the host
This should avoid interference between different containers and the host environment.
Fix devcontainer stuff
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 |
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 🤔 |
CC: tagging you in case you have time for a review @cclauss @eggplants @mwatts15 |
Can't likely do a full review, but I can try out basic project setup stuff
and verify, if that helps.
…On Mon, Jan 16, 2023, 13:10 Iwan Aucamp ***@***.***> wrote:
CC: tagging you in case you have time for a review @cclauss
<https://github.com/cclauss> @eggplants <https://github.com/eggplants>
@mwatts15 <https://github.com/mwatts15>
—
Reply to this email directly, view it on GitHub
<#2187 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALLFSCJWNZHJ2XQ3EMFSDDWSWMKZANCNFSM6AAAAAATLZ6IUA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@mwatts15 That will be appreciated if you have time. |
There was a problem hiding this 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
Thanks for the review @cclauss |
There was a problem hiding this 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.
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. |
Possibly a Poetry noob issue, but after I do a |
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! |
Thanks @mwatts15! Yes, *The removal of extras is buried (IMO) in
|
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:
***@***.***>
|
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
|
If you do I usually use pipx for poetry, as that then manages my venv for me, and then |
change `Black` -> `isort` in isort config.
Planning to merge a bit later tonight (CET). |
Summary
Goals:
setup.py
& setuptools topoetry
, for dependency and project management.setup.cfg
, intopyproject.toml
.pyproject.toml
, and remove the need for allrequirements*txt
files.Includes moving the package version into
pyproject.toml
, and in__init__.py
, reading the version frompyproject.toml
via use ofimportlib.metadata
.TODO
Fixes toDONEtox.ini
are still needed.Updates toDONEpyproject.toml
are needed, to add more dependency groups, which should then be referenced intox.ini
and other build/linting configs that have steps requiring development-dependencies.requirements*txt
files fromDONETaskfile.yaml
DONEworkflows/validate.yaml
RemoveDONErequirements*txt
files once they are no longer referenced anywhere.Other details
requirements*txt
files fromDockerfile.devcontainer
Checklist
the same change.
CHANGELOG.md
). (Elected not to updateCHANGELOG.md
since this PR does not have user-facing changes)Consideredgranting push permissions to the PR branch,so maintainers can fix minor issues and keep your PR up to date.