Migrate to pyproject.toml + cleanup

[Saransh-cpp]

• Zarr has a lot of redundant files in the root directory. The information contained in these files can be easily moved to pyproject.toml.
• Latest Python PEPs encourage users to get rid of setup.py and switch to pyproject.toml
• We should not be using setup.cfg until it is a necessity - Eventually deprecate setup.cfg with automatic conversion to pyproject.toml pypa/setuptools#3214
• We should not be using setuptools as a frontend (python setup.py install) - this is not maintained (confirmed by setuptools developers, but I cannot find the exact issue number at this moment)
• Zarr should switch to pre-commit.ci and remove the pre-commit workflow

I have tried to perform a 1:1 port. No extra information was added and all the existing metadata has been moved to pyproject.toml.

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[Saransh-cpp]

A fix for the failing docs - readthedocs/readthedocs.org#3322 (comment)

[Saransh-cpp]

Ideally, the requirements*.txt files should also be added as optional dependencies in pyproject.toml, but that would make this PR even bigger. Maybe I can carry that out in a follow-up PR. (That would also require editing documentation.)

[joshmoore]

Thanks for starting this discussion, @Saransh-cpp. I'm 👍 for use of pre-commit.ci and the readthedocs failure will need fixing in the RTD settings so the single test failure can be safely ignored for the moment.

I have tried to perform a 1:1 port. No extra information was added and all the existing metadata has been moved to pyproject.toml.

Thanks, this is a great way to keep the PR reviewable. There's one minor change from my side, but otherwise generally approve of moderization. 😄

Do any other @zarr-developers/python-core-devs have opinions here?

[Saransh-cpp]

Thanks for the quick review, @joshmoore! Should I go ahead and remove the Pre-commit-hooks workflow from zarr?

[joshmoore]

Should I go ahead and remove the Pre-commit-hooks workflow from zarr?

Yes, please, so that anyone who is reviewing will also be aware of the change.

[Saransh-cpp]

TODO for maintainers -

• Turn on pre-commit.ci for zarr

[lgtm-com]

This pull request fixes 6 alerts when merging 26a1e75 into 6ef11d3 - view on LGTM.com

fixed alerts:

• 4 for Module is imported more than once
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

[Saransh-cpp]

Thanks for the quick review, @jakirkham!

Would it make sense to break these over multiple lines as done below with other sections?

Unfortunately, tomli cannot parse blocks like -

readme = {
    file = "README.md", content-type = "text/markdown"
}

Hence, anything the metadata written in between { ... } should be written in a single line.

[lgtm-com]

This pull request fixes 6 alerts when merging 653f20a into f9581cb - view on LGTM.com

fixed alerts:

• 4 for Module is imported more than once
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

[jakirkham]

Will these perform the style fixes for us after pushing a PR? If so, that's great 😄

Though we probably want to document this behavior somewhere so users know about this. Maybe in the contributor guide?

[Saransh-cpp]

Yes! But the maintainers should install the precommit.ci app (https://github.com/marketplace/pre-commit-ci) for Zarr's GH repository.

I'll document it in the contributor guide.

[lgtm-com]

This pull request fixes 6 alerts when merging 45cd3dc into 2c52b17 - view on LGTM.com

fixed alerts:

• 4 for Module is imported more than once
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

[joshmoore]

8 Likes and no objections -- https://twitter.com/zarr_dev/status/1579838254379339776

Let's do it!

[DimitriPapadopoulos]

The LGTM.com analysis can be removed now that GitHub code scanning has been added:
The next step for LGTM.com: GitHub code scanning!

[DimitriPapadopoulos]

Why remove section [codespell] from setup.cfg?

Is it OK to put it back in pyproject.toml now that codespell supports it? See codespell-project/codespell#2290, included in latest codespell release 2.2.2. It's much better to maintain the list of false positives in pyproject.toml, so that it is used when running codespell from the command line.

[Saransh-cpp]

Zarr now uses codespell as a pre-commit hook 😉 which means that the spell check is automated and should be performed using pre-commit.

See -

zarr-python/.pre-commit-config.yaml

Lines 17 to 21 in 4362bb4

	- repo: https://github.com/codespell-project/codespell
	rev: v2.1.0
	hooks:
	- id: codespell
	args: ["-L", "ba,ihs,kake,nd,noe,nwo,te"]

[DimitriPapadopoulos]

I see. You could maintain codespell options in pyproject.toml and run it as a pre-commit hook nevertheless.

At some point, I expect a false positive to be introduced in new code. I feel that being able to run codespell directly, with the same options, will help understand and fix the problem.

[joshmoore]

Seems if the options can be in pyproject.toml rather than .pre-commit-config.yaml that's optimal.

The LGTM.com analysis can be removed now that GitHub code scanning has been added:

@DimitriPapadopoulos: just to be clear, I'll remove the app, yeah?

[DimitriPapadopoulos]

@DimitriPapadopoulos: just to be clear, I'll remove the app, yeah?

See #1191.

[DimitriPapadopoulos]

Seems if the options can be in pyproject.toml rather than .pre-commit-config.yaml that's optimal.

Yes, but I need to fix a few issues I have with codespell first, to be able to test with the latest version 2.2.2, which supports pyrpoject.toml.

[Saransh-cpp]

IMO it would be better to keep the configuration of all the pre-commit hooks at one place - inside .pre-commit-config.yaml. You can always do -

pre-commit run codespell --all-files

to run codespell on all files.

[DimitriPapadopoulos]

The point is that codespell is not necessarily run from pre-commit. I mean, that's your point of view, but my point of view is that it should be possible to run codespell from the command line as usual.

[joshmoore]

@Saransh-cpp: I'd guess I'd counter with how much can be inside the pyproject.toml? :) @DimitriPapadopoulos has really been leading the lint-ification and so if we can keep the "running without pre-commit" use case, that'd be great.

[Saransh-cpp]

Yes, running it without pre-commit sounds good too! Any path that would make the process smoother!

[jakirkham]

Would be good to make the same changes to Numcodecs 🙂

Filed as issue ( zarr-developers/numcodecs#364 )

[DimitriPapadopoulos]

Also, I do not have much experience with pre-commit, but it doesn't work for me on Ubuntu 22.04:

$ pre-commit run codespell --all-files
[INFO] Installing environment for https://github.com/codespell-project/codespell.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/python3', '-mvirtualenv', '/home/username/.cache/pre-commit/repolbk82i8v/py_env-python3.9', '-p', 'python3.9')
return code: 1
expected return code: 0
stdout:
    RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.9'
    
stderr: (none)
Check the log at /home/username/.cache/pre-commit/pre-commit.log
$ 

I am probably doing something stupid here.

[mzjp2]

Do you have Python 3.9 installed?

[DimitriPapadopoulos]

No, I work with Python 3.10.

[mzjp2]

That'll be why it's failing - we should probably loosen the pre-commit language version to just python3.

[DimitriPapadopoulos]

Yes, having to install Python 3.9 just to run codespell is really too complex.

This seems to be a default version suggestion, isn't it possible to override it?

zarr-python/.pre-commit-config.yaml

Lines 5 to 6 in 4362bb4

	default_language_version:
	python: python3.9

As a side note, pre-commit should fail more gracefully, the error message doesn't make any sense. I'l try to open an issue if I can find the time...

[mzjp2]

Yep, just change it to python3 instead.

[jakirkham]

This was changed to python3 along with a few other fixes that were need in PR ( #1197 )

[jakirkham]

The LGTM.com analysis can be removed now that GitHub code scanning has been added:
The next step for LGTM.com: GitHub code scanning!

Done in PR ( #1191 )

[joshmoore]

seealso: #1209