-
-
Notifications
You must be signed in to change notification settings - Fork 577
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
Switch to hatchling backend #4747
Switch to hatchling backend #4747
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4747 +/- ##
========================================
Coverage 98.69% 98.69%
========================================
Files 303 303
Lines 23256 23256
========================================
Hits 22953 22953
Misses 303 303 ☔ View full report in Codecov by Sentry. |
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.
Hi @vidipsingh, thanks for taking this up! I have just a few suggestions. Please remove other references of setuptools
from this file (lines 174–189) and across other files as well, and replace them with hatch
/hatchling
as necessary.
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.
There seems to be a lot of changes here that are not necessary for just switching to hatch. It should only be a couple lines.
Switching to hatch also requires changing the release workflow, the update version script, and other tools. There is a bunch of other stuff that needs to change
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.
Agree with @kratman. Switching to hatchling
should be one PR, and adding hatch-vcs
should be another. @vidipsingh can you restrict this PR to just switching to hatchling
and getting rid of setuptool
configurations?
The hatch-vcs
PR can deal with updating the release workflow. Switching to hatchling
will not change the release workflow.
In any case, both these PRs should go in after 25.1
@Saransh-cpp Okay, I will restrict this PR to just switching to I also wanted to ask where else I need to remove references to
Since I’m new to the PyBaMM repo, any guidance or suggestions regarding these changes would be greatly appreciated :) |
Sounds right! Some of the occurrences might have to be preserved, but we can point that out while reviewing the PR. |
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, @vidipsingh, looks good to me now!
Could you please rename the PR title, and replace the "Fixes" keyword in the PR description with "Related to" or similar so that the linked issue doesn't get closed, as we are no longer adding |
@all-contributors please add @vidipsingh for infra |
I've put up a pull request to add @vidipsingh! 🎉 |
Actually, wait, I'm dismissing my review because the references to setuptools across other files still remain: #4747 (comment)
pyproject.toml
Outdated
"src/pybamm/**/*.csv", | ||
"src/pybamm/**/*.py", | ||
"src/pybamm/CITATIONS.bib", | ||
"src/pybamm/plotting/mplstyle", |
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 don't see this file in the repo
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.
See my comment below.
pyproject.toml
Outdated
include = [ | ||
"src/pybamm/**/*.txt", | ||
"src/pybamm/**/*.md", | ||
"src/pybamm/**/*.csv", | ||
"src/pybamm/**/*.py", | ||
"src/pybamm/CITATIONS.bib", | ||
"src/pybamm/plotting/mplstyle", |
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.
The comment explaining why this is not automatically generated is left below when the code was moved.
It would be nice if we could switch to this being automatic though
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.
Makes sense, it's automatic for setuptools if include_package_data
is set to True
. Could you check if a similar option is available for hatchling, @vidipsingh?
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.
It is not automatic for setuptools (you need to specify files in MANIFEST.in
, as we do right now), but it is automatic for hatch 😉
See my comment below.
pyproject.toml
Outdated
# List of files to include as package data. These are mainly the parameter CSV files in | ||
# the input/parameters/ subdirectories. Other files such as the CITATIONS file, relevant | ||
# README.md files, and specific .txt files inside the pybamm/ directory are also included. | ||
# These are specified to be included in the SDist through MANIFEST.in. |
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.
# List of files to include as package data. These are mainly the parameter CSV files in | |
# the input/parameters/ subdirectories. Other files such as the CITATIONS file, relevant | |
# README.md files, and specific .txt files inside the pybamm/ directory are also included. | |
# These are specified to be included in the SDist through MANIFEST.in. |
pyproject.toml
Outdated
[tool.hatch.build] | ||
packages = ["src/pybamm"] | ||
include = [ | ||
"src/pybamm/**/*.txt", | ||
"src/pybamm/**/*.md", | ||
"src/pybamm/**/*.csv", | ||
"src/pybamm/**/*.py", | ||
"src/pybamm/CITATIONS.bib", | ||
"src/pybamm/plotting/mplstyle", |
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.
include-package-data
was used to include the files from MANIFEST.in
in the wheel file.
hatchling
includes everything not ignored by .gitignore
by default. Therefore, we should just include the required files -
[tool.hatch.build] | |
packages = ["src/pybamm"] | |
include = [ | |
"src/pybamm/**/*.txt", | |
"src/pybamm/**/*.md", | |
"src/pybamm/**/*.csv", | |
"src/pybamm/**/*.py", | |
"src/pybamm/CITATIONS.bib", | |
"src/pybamm/plotting/mplstyle", | |
[tool.hatch] | |
build.targets.sdist.include = [ | |
"src/pybamm", | |
"CITATION.cff", | |
] |
We should not bundle the CITATION.cff
file with the wheel (as it is being done now). We should also get rid off MANIFEST.in
.
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.
@vidipsingh This change is still needed
@vidipsingh Can you resolve the conflicts here? |
- Replace setuptools with hatchling as build backend - Add hatch-vcs for Git tag-based versioning - Unify package versioning under a single source
- Replace setuptools with hatchling as build backend - Remove setuptools configurations - Revert to version 24.11.2
- Replace setuptools with hatch as build backend - Remove setuptools configurations - Add automatic file handling in hatch config
- Replace setuptools with hatchling as build backend - Add hatch-vcs for Git tag-based versioning - Unify package versioning under a single source
- Replace setuptools with hatchling as build backend - Remove setuptools configurations - Revert to version 24.11.2
- Replace setuptools with hatch as build backend - Remove setuptools configurations - Add automatic file handling in hatch config
53cda20
to
b3a5de5
Compare
@vidipsingh As a general rule, it is best not to force push once the review has started. It makes it really hard to see the changes. We squash most PRs (except releases), so the merge commits do not matter |
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.
It looks like you might have reverted a bunch of stuff from the previous reviews
…m/vidipsingh/PyBaMM into switch-to-hatchling-and-hatch-vcs
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 this is looking good now. The failing test is a random failure, it is unrelated
Changes were made, looks like all review comments were covered
@Saransh-cpp, @agriyakhetarpal I checked all the previous comments, and they seem to be covered. So this should be ready to go |
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.
There are a few other references to setuptools
that should be removed as well:
- https://github.com/vidipsingh/PyBaMM/blob/adc7e0c3670ad2bf9571ff4f02d8472dda874ed3/pyproject.toml#L155-L170
MANIFEST.in
, which was used bysetuptools
to configure what goes in the sdist, is now irrelevant and can be deleted
Happy to approve after, thanks!
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, @vidipsingh! Looks good to me!
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.
@agriyakhetarpal We just need your review now
Thanks @Saransh-cpp, @kratman, and @agriyakhetarpal for reviewing and merging my PR! Much appreciated :) |
Thanks for your work! Please feel free to work on adding support for |
Sure! I'll work on the |
Description
This PR switches PyBaMM's build system from setuptools to hatchling and implements Git tag-based versioning using hatch-vcs. This change simplifies our build process and version management now that PyBaMM is a pure Python package.
Related to: #4742
Type of change
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
)$ python -m pytest
(or$ nox -s tests
)$ python -m pytest --doctest-plus src
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks: