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

Switch build backend to hatchling #12173

Closed

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Nov 5, 2023

Fixes #12169

  • Switching to the standardized build process (I tested with the simple build frontend to begin with, before moving on to pip) forces us to let go of some setuptools-specific implementation details. Specifically, we have to say goodbye to MANIFEST.in and dynamic dependency generation in setup.py – which is a good thing! Dependencies have to be specified inside of pyproject.toml
  • I moved the dependencies specified in requirements_base.txt to project.dependencies (no behavior change when pip-installing mne)
  • I moved the dependencies specified in requirements_hdf5.txt to project.optional-dependencies.hdf5 (no behavior change when pip-installing mne[hdf5])
  • I moved the dependencies specified in requirements.txt to project.optional-dependencies.full (new option: pip install mne[full])

This allowed me to delete those requirements files. They were only containing abstract dependencies; hence, pyproject.toml is the better place for those dependency specifications anyway.

(btw there were also inconsistencies e.g. between the NumPy versions listed in requirements_base.txt and requirements.txt)

  • As for the [test] variant we currently have – I found this isn't documented anywhere. Instead, all our contribution docs state that requirements_testing.txt should be used. To keep friction as low as possible, I kept requirements_testing.txt and requirements_testing_extra.txt, but removed the mne[test] variant in order not to have to duplicate dependency specifications.

  • For the doc requirements, I also didn't change any of the existing behavior (no new variant; we stick with requirements_doc.txt)

  • After these changes were complete, I could remove setup.py.

  • generate_codemeta.py needs to be updated – I don't really know what this is used for and what the best approach here would be, but since I removed setup.py and requirements.txt, these lines will certainly not work anymore:

    # GET OUR DEPENDENCY VERSIONS
    with open(out_dir / "setup.py", "r") as fid:
    for line in fid:
    if line.strip().startswith("python_requires="):
    version = line.strip().split("=", maxsplit=1)[1].strip("'\",")
    dependencies = [f"python{version}"]
    break
    with open(out_dir / "requirements.txt", "r") as fid:
    for line in fid:
    req = line.strip()
    for hard_dep in hard_dependencies:
    if req.startswith(hard_dep):
    dependencies.append(req)

  • The changes to .git_archival.txt shall ensure that VCS version detection also works when downloading an archive of the repository (maybe some of you remember that this used to be a problem after we switched to using setuptools-scm)

💬 Any feedback would be greatly appreciated. Please test as "regular" install; as editable install; install from sdist; install from wheel. Thank you! 😊

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

why did you delete the manifest? note that data files we have in the package for testing cannot be part of the source package otherwise it's too big.

Do we really have to live on the edge? setuptools is really the standard and if it has problem why not fix it rather coming up with a new way that much fewer people are likely to know and are able to maintain? #grumpy agramfort ;)

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 5, 2023

why did you delete the manifest? note that data files we have in the package for testing cannot be part of the source package otherwise it's too big.

Yes I checked, these files are excluded :) I will check again just to be sure.

Do we really have to live on the edge? setuptools is really the standard and if it has problem why not fix it rather coming up with a new way that much fewer people are likely to know and are able to maintain? #grumpy agramfort ;)

It's just an experiment for now to see if it works and how much change it would require. Like I said in #12169 I can also live with doing pip install --config-settings editable-mode=strict

But also note that in the medium-to-long-run, we will have to move away from setup.py and MANIFEST.in, as this is now considered legacy

Moving the dependencies into pyproject.toml is a good idea in either case

@hoechenberger
Copy link
Member Author

But also note that in the medium-to-long-run, we will have to move away from setup.py and MANIFEST.in, as this is now considered legacy

Moving the dependencies into pyproject.toml is a good idea in either case

That said, it will be easier to maintain a couple years down the road, because this is now the standardized way of doing things

@cbrnr
Copy link
Contributor

cbrnr commented Nov 6, 2023

I am very much in favor of moving completely to pyproject.toml, because this is the current standard way to do it (it's not bleeding edge anymore).

However, I would rethink our current extra dependencies. In particular, I don't like that we still keep some requirements.txt files around. Instead, I suggest that we add a new [dev] extra dependency, which includes everything for development (i.e., the previous test and doc extras).

We could also remove the [hdf5] extra, because it contains only two packages anyway. I would either include that in the base requirements, or in [full]. Let's try to make it as simple as possible.

Regarding Alex's comment about MANIFEST.in and package data, this should now work without this legacy file (it was only required by Setuptools). However, we really need to test this very carefully! The beauty about having everything in pyproject.toml is that we can switch backends without a single change (at least in theory, currently, each backend might need a section with specific parameters).

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 6, 2023

How about this:
I make a new PR where I only get rid of the user (not dev) requirements files, setup.py, and MANIFEST.in, but we don't change the build backend just yet. Then we can see if things are still working as expected, and only later on decide whether we want to move to hatchling or not.

I'd also like to emphasize that Hatch and hatchling are developed by the PyPA team and they use it for example for Black. So it's not just a fancy new experimental thing, but a tool developed and used by the group that makes and maintains the packaging standards.

As for moving dev and doc build dependencies into pyproject.toml as well, this is ultimately a matter of taste, I would say. Esp for the doc deps, these are rather "concrete" (explicit installation of dev versions of many packages, directly from their git repo), hence I'm leaning toward leaving them in a requirements file. And for the testing deps, if we move them to pyproject.toml, the workflow for developers changes. I didn't dare touch it for this reason... Black also has the dev deps only in a requirements file. In the end, again, it's probably a matter of taste. (For NPM projects, you always list dev deps in a separate section inside package.json, which I actually always liked)

@sappelhoff
Copy link
Member

why did you delete the manifest? note that data files we have in the package for testing cannot be part of the source package otherwise it's too big.

Yes I checked, these files are excluded :) I will check again just to be sure.

How does that work now? I don't see new added lines for that in the diff 🤔

In general In am in favor of this move. Especially, because I agree with:

moving completely to pyproject.toml, because this is the current standard way to do it (it's not bleeding edge anymore

and

Hatch and hatchling are developed by the PyPA team and they use it for example for Black. So it's not just a fancy new experimental thing, but a tool developed and used by the group that makes and maintains the packaging standards.

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 6, 2023

In general In am in favor of this move. Especially, because I agree with:

moving completely to pyproject.toml, because this is the current standard way to do it (it's not bleeding edge anymore

and

Hatch and hatchling are developed by the PyPA team and they use it for example for Black. So it's not just a fancy new experimental thing, but a tool developed and used by the group that makes and maintains the packaging standards.

FWIW they even use hatchling as the default build backend in their tutorials:
https://packaging.python.org/en/latest/tutorials/packaging-projects/#choosing-a-build-backend

why did you delete the manifest? note that data files we have in the package for testing cannot be part of the source package otherwise it's too big.

Yes I checked, these files are excluded :) I will check again just to be sure.

How does that work now? I don't see new added lines for that in the diff 🤔

For file inclusion: by default, now pretty much everything inside the package dir is included automatically. There are some exceptions and our testing datasets seem to be among those. We can make it explicit, though. Exclusions can be specified like here:

https://github.com/mne-tools/mne-python/pull/12173/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R114

CI error shows that I'm still failing to include some files. We just have to use the include directive to fix this:
https://hatch.pypa.io/latest/config/build/#patterns

@hoechenberger hoechenberger added the needs-discussion issues requiring a dev meeting discussion before the way forward is clear label Nov 6, 2023
@cbrnr
Copy link
Contributor

cbrnr commented Nov 6, 2023

For file inclusion: by default, now pretty much everything inside the package dir is included automatically. There are some exceptions and our testing datasets seem to be among those. We can make it explicit, though.

👍 for making exclusions explicit.

@sappelhoff
Copy link
Member

Thanks!

We can make it explicit, though.

IMHO, explicit would be more transparent and readable.

@larsoner
Copy link
Member

larsoner commented Nov 6, 2023

I don't mind trying out hatchling, especially since it's developed by PyPA and used by Black already. I think as long as all the right stuff is included in the source tarball and wheel -- and we can check these with directory diffs on main vs this PR for example -- then we're going to be okay.

But @hoechenberger it might indeed be easier to start out with a first PR that pushes us toward pyproject.toml specification of requirements rather than requirements.txt-type files. Then a follow-up PR could use hatchling. Up to you.

However, I would rethink our current extra dependencies.

I don't think we should do that here -- @hoechenberger has done a good job so far of keeping the current PR backward compatible. Things like removing hdf5 are mostly orthogonal and require discussion. Feel free to reopen #10219 if you want to propose a structure keeping in mind the other stuff brought up in that issue.

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 6, 2023

@larsoner Will do.

Do you have any opinion regarding the dev dependencies? Would you rather see them added to pyproject.toml too, or keep them separate in requirements files?

What about the current mne[tests] variant?

@drammock
Copy link
Member

drammock commented Nov 6, 2023

Do you have any opinion regarding the dev dependencies? Would you rather see them added to pyproject.toml too, or keep them separate in requirements files?

IMO the end result should be no requirements*.txt files left at all. Note that the optional dependencies sections in pyproject.toml can be hierarchical, e.g., the "dev" section can be something like

mne[test], mne[doc], mne[full]

@larsoner
Copy link
Member

larsoner commented Nov 6, 2023

Works for me @drammock !

@hoechenberger
Copy link
Member Author

hoechenberger commented Nov 6, 2023

Do you have any opinion regarding the dev dependencies? Would you rather see them added to pyproject.toml too, or keep them separate in requirements files?

IMO the end result should be no requirements*.txt files left at all. Note that the optional dependencies sections in pyproject.toml can be hierarchical, e.g., the "dev" section can be something like


mne[test], mne[doc], mne[full]

Yes, I actually came up with this solution on my first iteration but didn't dare push it for fear of too much change (and too little time for proper change management on my side) 😅😅
But great that you agree

I've got something to work with now.

@larsoner larsoner added this to the 1.7 milestone Nov 7, 2023
@larsoner
Copy link
Member

larsoner commented Dec 1, 2023

@hoechenberger how about we try this out with mne-bids-pipeline for a few months first? Then if all is good there then we switch MNE-Python over, too.

@hoechenberger
Copy link
Member Author

Yes why not

I'm already using hatchling with mne-python-stubs and it works beautifully, maybe this can serve as a starting point

https://github.com/hoechenberger/mne-python-stubs/blob/dfea055c83172096feb6ac833cb4cfc3c4c634c4/pyproject.toml#L2-L3

@ofek
Copy link

ofek commented Dec 3, 2023

Maintainer of Hatch/Hatchling here! Let me know if I can be of assistance 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion issues requiring a dev meeting discussion before the way forward is clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editable installs with pip and VS Code
7 participants