-
Notifications
You must be signed in to change notification settings - Fork 5
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
update pyproject.toml to use pdm instead of poetry #236
base: main
Are you sure you want to change the base?
update pyproject.toml to use pdm instead of poetry #236
Conversation
NOTE: - Upgrade required python from 3.9 to 3.10 - Upgrade scipy from 1.11 to 1.15 - Upgrade numpy from 1.26 to 2.2 - Change "documentation" url from readthedocs to github pages
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.
Amazing! I'm looking forward to getting this over to PDM 😄 I see this is a draft, but I added some inline comments still
homepage = "https://www.nb.no/dh-lab/" | ||
documentation = "https://dhlab.readthedocs.io/en/stable/" | ||
repository = "https://github.com/NationalLibraryOfNorway/DHLAB" | ||
requires-python = "<3.13,>=3.10" |
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.
requires-python = "<3.13,>=3.10" | |
requires-python = ">=3.10" |
I don't believe we need the pin on the upper version number with PDM or uv?
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.
We do - the library breaks with numpy>=2 and scipy>=1.15, because of the legacy code that is imported at the top level __init__.py
. When I tried to restrict only numpy and scipy versions, pdm complained that the python version range was incompatible with the dependencies.
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.
I see, we should probably fix that at one point, but that's a separate issue :)
"scipy==1.14", | ||
"openpyxl>=3.1.2", | ||
"beautifulsoup4>=4.12.2", | ||
"jinja2>=3.1.4", | ||
"tqdm>=4.66.6", | ||
"numpy==1.26", |
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.
"scipy==1.14", | |
"openpyxl>=3.1.2", | |
"beautifulsoup4>=4.12.2", | |
"jinja2>=3.1.4", | |
"tqdm>=4.66.6", | |
"numpy==1.26", | |
"scipy>=1.14", | |
"openpyxl>=3.1.2", | |
"beautifulsoup4>=4.12.2", | |
"jinja2>=3.1.4", | |
"tqdm>=4.66.6", | |
"numpy>=1.26", |
It would be nice not to have a hard pin on old versions of numpy and scipy
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.
I absolutely agree - but ref. the comment above (https://github.com/NationalLibraryOfNorway/DHLAB/pull/236/files#r1927195453), the library breaks with higher versions.
] | ||
|
||
[build-system] | ||
requires = ["pdm-backend"] | ||
build-backend = "pdm.backend" |
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.
We should either
- use a
src
-layout, placing thedhlab
-directory inside asrc
-directory, - or specify that we have a flat layout for the build backend (see the pdm documentation here).
I'm leaning towards 1. since it would make it easier for tests to pick up on installation errors. This also seems to be the prefered layout these days (see e.g. the tutorial on packaging.python.org)
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.
Good suggestion!
Co-authored-by: Marie Roald <[email protected]>
Additional changes
Upgrade scipy from 1.11 to 1.15Upgrade numpy from 1.26 to 2.2.github/workflows/CI.yml
uses pdm instead of poetry