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

Pyproject.toml & setuptools_scm support #346

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

joshmoore
Copy link
Member

@joshmoore joshmoore commented Jan 17, 2024

As a follow on from #325, this moves ome-zarr-py to the use of setuptools_scm (like ome/omero-cli-zarr#159) so that tags with their releases can be created directly from the GitHub UI. cc: @dstansby

To simplify this, I've gone ahead and migrated setup.py to pyproject.toml. I imagine this is a template that we can follow elsewhere. The major discrepancy in the mapping between the two was that test_requires is now an optional dependencies key which may require some adjustments.

Major question from my side is which python version to pin to. Note that zarr-python has dropped 3.8 for the next release and there is already discussion about dropping 3.9 too based on SPEC-0000.

A minor question is whether or not we still want to write the version number to a file or to just leave folks to use importlib.metadata.version("ome_zarr").

@joshmoore
Copy link
Member Author

Still needs a replacement for bsdist & wheel creation, e.g., https://stackoverflow.com/questions/58753970/how-to-build-a-source-distribution-without-using-setup-py-file

@joshmoore
Copy link
Member Author

Trying to get this green to see if it addresses @akhanf's issues in #353.

@joshmoore
Copy link
Member Author

Reviewers requested, largely with an OR.

pyproject.toml Outdated Show resolved Hide resolved
@will-moore
Copy link
Member

Re: minor question - we haven't previously written the version number to a file, right?

@will-moore
Copy link
Member

Reviewers requested, largely with an OR.

What's an OR?

@joshmoore
Copy link
Member Author

Sorry, I mentioned recently elsewhere to Seb, that I'm not expecting all of these reviewers (i.e. with an AND), but any of you would be great (OR).

@joshmoore
Copy link
Member Author

Re: minor question - we haven't previously written the version number to a file, right?

I'm not sure what you're asking. Previously, we've hard-coded the version in setup.py: zarr-developers/zarr-developers.github.io#105 (comment)

@will-moore
Copy link
Member

Ah, understood. With "write the version number to a file" I was thinking of the functionality like in omero-cli-zarr where the version is importable by the code and is written to a file (https://github.com/ome/omero-cli-zarr/blob/75ab185a184410aa408772e12315ba9cbf209b36/src/omero_zarr/raw_pixels.py#L350).
But in ome-zarr-py we've never needed the ability for the code to access the version (Could you previously import it from setup.py?).
If you can get it with importlib.metadata.version("ome_zarr") anyway then 👍

@will-moore
Copy link
Member

With this branch...

pip install -e .
...
Successfully built ome-zarr
Installing collected packages: ome-zarr
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
napari-ome-zarr 0.5.2 requires ome-zarr>=0.3.0, but you have ome-zarr 0.0.0 which is incompatible.
omero-cli-zarr 0.5.5.dev10+g893d4ee requires ome-zarr>=0.5.0, but you have ome-zarr 0.0.0 which is incompatible.
Successfully installed ome-zarr-0.0.0

Tried getting version number as described:

>>> import importlib
>>> importlib.metadata.version("ome_zarr")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'importlib' has no attribute 'metadata'

After googling alternative approach:

>>> import pkg_resources
>>> pkg_resources.get_distribution("ome_zarr").version
'0.0.0'

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

At the code level, the changes look reasonable.

On the build system, I am not strongly opinionated about either form and would need to read more to have an informed view. My primary feedback is practical as updating the style/layout can introduce some churn with effectively zero value for end-users and additional burden for maintainers e.g. when looking at the code history. This is mostly related to the suggestion that the template could be followed elsewhere. Only suggestion is that I would weigh the impact before rolling out the new style to all OME Python repositories.

On the bumpversion vs setuptools-scm (or similar) workflow, it all boils down to the usual trade-off between a quick release workflow via a tag only vs managing the version lifecycle explicitly. Here again, I don't have a strong opinion as we already have a mixture of workflows. The biggest caveats of the setuptools-scm approach that I remember is the requirement for a Git checkout and also tags (esp. for forks).
The readme section about the release process should likely be updated accordingly.

Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I see a ping to me from many months ago! This looks great to me, and as well as hopefully helping with #353 would also help for folks using uv (including myself), that needs a pyproject.toml to read from.

On Python support, I think SPEC-0 forces that hand of any package that depends on numpy (or other scientific python packages) to follow it, so I'd advocate for >=3.11 here.

"numpy",
"dask",
"distributed",
"zarr>=2.8.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"zarr>=2.8.1",
"zarr>=2.8.1,<3",

requires-python = ">3.8"

dependencies = [
"tifffile",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"tifffile",

This doesn't seem to be an actual dependency?

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.

4 participants