🔧 Move to flit for PEP 621 compliant package build #5312

chrisjsewell · 2022-01-15T06:26:59Z

https://github.com/pypa/flit is now officially part of the packaging organization.
It is a PEP 621 (pyproject.toml only build specification) and, since v3.4, PEP 660 (pip install --editable . builds with pyproject.toml only) compliant build tool for pure-python packages.

Restricting itself to pure-python packages (which is all we need), has meant it has been able to progress a lot more rapidly than setuptools, which is stuck supporting legacy issues and more complex builds (e.g. compiling C extensions) (see pypa/setuptools#1688).
I believe flit will become the de-facto builder for new (simple) packages, and anyhow because it is PEP compliant, you can simply swap it out at any point.

Very simply, I have just migrated everything from setup.json into pyproject.toml, and removed setup.json, setup.py and MANIFEST.in.
The package version is sourced directly from aiida/__init__.py, and so there is no need for any consistency checks. (also with aiidateam/aiida-registry#201, these versions will now still show in the aiida-registry)

Notes:

to use pip install -e ., this requires pip v21, but then it "just works"
the fastentrypoints build requirement has been removed, since with flit (and also python 3.8 + setuptools) the issue it solves is gone
this removes the dynamically generated all extra, and the logic to append rest+atomic_tools to the tests and docs extras. This was in the setup.py, but anyway I feel it was unintuitive.
I'd also note https://github.com/aiidateam/aiida-firecrest is already using this build system 😄 (and it's working great)

codecov · 2022-01-15T07:26:51Z

Codecov Report

Merging #5312 (a2f57cd) into develop (18f6c9a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #5312   +/-   ##
========================================
  Coverage    82.02%   82.02%           
========================================
  Files          533      533           
  Lines        38255    38255           
========================================
  Hits         31374    31374           
  Misses        6881     6881

Flag	Coverage Δ
django	`77.07% <ø> (-<0.01%)`	⬇️
sqlalchemy	`76.36% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18f6c9a...a2f57cd. Read the comment docs.

chrisjsewell · 2022-01-16T13:32:26Z

If you are particularly interested in packaging developments, you can also read: https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html

chrisjsewell · 2022-01-16T17:48:31Z

another note:
you can actually "recover" the pip install .[all] functionality by instead using flit install --extras=all (there is an open issue about allowing this with pip pypa/flit#488)
(also I opened an issue about flit install --editable pypa/flit#512)

csadorf

Great! I was looking forward to an overhaul of our build setup for quite a while.

One comment regarding flit. I agree that this looks very promising and since adopting flit does not have any impact on the user installation flow, absolutely zero reservations. And if it is a sensible and well maintained tool, I hope that it gains traction. But the fact that it is now maintained under umbrella of the Python Packing Authority is not necessarily a sufficient indicator for success, because so is pipenv.

I have a few comments, suggestions, and one change request.

.github/workflows/check_release_tag.py

pyproject.toml

utils/requirements.txt

chrisjsewell · 2022-01-17T09:40:03Z

And if it is a sensible and well maintained tool, I hope that it gains traction. But the fact that it is now maintained under umbrella of the Python Packing Authority is not necessarily a sufficient indicator for success, because so is pipenv.

As mentioned, it does not really matter that much if it did fail, because it is PEP 621 compliant (this is different to pipenv, which has no such compliance backing), so you simply swap out the build tool for another; only having to change two lines in pyproject.toml and everything will still just work.
(But I am happy to bet money that it will be successful as many packages have already begun to use it 😉 )

Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>

csadorf · 2022-01-17T10:22:14Z

And if it is a sensible and well maintained tool, I hope that it gains traction. But the fact that it is now maintained under umbrella of the Python Packing Authority is not necessarily a sufficient indicator for success, because so is pipenv.

As mentioned, it does not really matter that much if it did fail, because it is PEP 621 compliant (this is different to pipenv, which has no such compliance backing), so you simply swap out the build tool for another; only having to change two lines in pyproject.toml and everything will still just work.

Yes, indeed, hence no reservations.

(But I am happy to bet money that it will be successful as many packages have already begun to use it 😉 )

(I'm not going to bet against that.)

sphuber

Thanks @chrisjsewell . If these changes indeed do not change the impact of the install procedure for users, then this is fine with me as well. I have three questions though.

sphuber · 2022-01-17T10:53:23Z

.github/workflows/check_release_tag.py

+
+
+def get_version_from_module(content: str) -> str:
+    """Get the __version__ value from a module."""


I take it that you employ this (convoluted) way of reading the version number because you cannot import it normally? Instead of this approach, is there no simple way to make the package importable so we can just do

import aiida version = aiida.__version__

At the very least, I think it would be useful to have a comment here explaining why this approach is necessary and the import way doesn't work or is not desirable

I wouldn't call this approach particularly convoluted; I would say having to go through the whole process of installing/importing the package is convoluted, just to read the version from a file 🤷

Feel free to create a suggestion for what the docstring should be (and I'll happily merge), but I'm not adding one myself, because I don't feel it needs it, since this IMO is the canonical way to retrieve the __version__ attribute (it is how both setuptools and flit do it)

If generating an abstract syntax tree, parsing it element by element just to find the one string that defines the version number is not convoluted, then I don't know what is. I can see the point that having to install it is likewise a lot of work just to get the version, but at least it is very understandable and readable. One would think that there should be an easier way to do this... But in the absence of that, guess this is fine to keep

@sphuber It is convoluted, but to my understanding, one main motivation for the overall change is to make the project metadata and build definition more declarative, so needing to install and import the package runs exactly counter that. I believe that we can reduce complexity by either parsing the module directly with a regex or by moving to a different version specification like I suggested in my comment. My recommendation would be to move forward and avoid bikeshedding on this issue.

I already agreed to keeping this and move forward. Still, the suggestion to add a comment explaining why this is being done doesn't seem like a big or weird ask. Even if this may be the standard, I doubt many developers are aware of this and they will almost certainly wonder why it has to be (or look) so complicated.

I would note, that on the next line, it says: # adapted from setuptools/config.py, but yeh honestly this is just a very simple check (that wasn't even here a year ago) to make sure the release tag is consistent with the package version

sphuber · 2022-01-17T10:54:05Z

.github/workflows/check_release_tag.py


 if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument('GITHUB_REF', help='The GITHUB_REF environmental variable')
-    parser.add_argument('SETUP_PATH', help='Path to the setup.json')
+    parser.add_argument('INIT_PATH', help='Path to the aiida/__init__.py file')


Does this deserve an option? Are we ever going to change the location? Would simplify this to being hardcoded

ok removed 👍

sphuber · 2022-01-17T10:59:44Z

pyproject.toml

+"core.arithmetic.multiply_add" = "aiida.workflows.arithmetic.multiply_add:MultiplyAddWorkChain"
+"core.arithmetic.add_multiply" = "aiida.workflows.arithmetic.add_multiply:add_multiply"
+
+[tool.flit.module]


How and where is the functionality of the MANIFEST.in taken care off?

flit already handles all this out-the-box, see e.g. https://flit.readthedocs.io/en/latest/pyproject_toml.html#sdist-section.
If you want to check what is included in the sdist and wheel, feel free to try running flit build and exploring the distributions yourself. I've already done this, though, and can confirm that all package_datais correctly in the wheel and sdist.

How does it know what files to include though? Does it simply include all files within the aiida folder? The docs you link say:

When you use flit build or flit publish, Flit builds an sdist (source distribution) tarball containing the files that are checked into version control (git or mercurial).

That seems to suggest to me that it would also include the tests folder for example. Is that true? Because I don't think that is desirable. This would make the package unnecessarily heavy, and we intentionally moved the tests outside of the package such that they don't have to be shipped with the build.

Well firstly, just below this line you'll see:

[tool.flit.sdist] exclude = [ "docs/", "tests/", ]

So currently, the docs and tests are not added to the sdist anyway.
But actually, this does not really matter, since both the sdist AND wheel are built/uploaded to PyPI.
The wheel (see https://www.python.org/dev/peps/pep-0491/) is the thing you actually install with pip (if available from pypi), and that does not contain anything from outside the module:

(there is some extra discussion about this in aiidateam/aiida-registry#201)

chrisjsewell · 2022-01-17T14:04:39Z

cheers guys!

- Move to flit for PEP 621 compliant build (see also aiidateam/aiida-core#5312) - Remove `setup.py`,` setup.json`, `MANIFEST.in`, `check_version.py`, and `.coveragerc` - Set `requires-python = ">=3.7"` - Move from `yapf` to `black` for code formatting - Add `isort` and `pyupgrade` pre-commit hooks - Move documentation theme from `sphinx-rtd-theme` to `furo` - Add `tox` configuration (in `pyproject.toml`) Note the setuptools `reentry_register` hook is no longer possible, although reentry is anyway removed in AiiDA v2.

chrisjsewell added 6 commits January 15, 2022 07:26

🔧 Move to flit for package build

afd74c7

Update .pre-commit-config.yaml

63e28f0

update dependency checks

25c6a25

Update requirements.txt

5e22916

Update dependency_management.py

f575508

replace occurrences of setup.json

3eb9224

chrisjsewell changed the title ~~🔧 Move to flit for package build~~ 🔧 Move to flit for PEP 621 compliant package build Jan 15, 2022

Update release.yml

5ad0043

chrisjsewell marked this pull request as ready for review January 15, 2022 07:48

chrisjsewell requested review from csadorf, sphuber and ltalirz January 15, 2022 07:48

Merge branch 'develop' into flit

b8da438

chrisjsewell added 3 commits January 17, 2022 06:32

Update pyproject.toml

2071695

Merge branch 'develop' into flit

8322c98

Update release.yml

409599b

csadorf suggested changes Jan 17, 2022

View reviewed changes

.github/workflows/check_release_tag.py Show resolved Hide resolved

pyproject.toml Show resolved Hide resolved

pyproject.toml Show resolved Hide resolved

utils/requirements.txt Outdated Show resolved Hide resolved

chrisjsewell and others added 2 commits January 17, 2022 10:50

Update utils/requirements.txt

1dccb26

Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>

Merge branch 'develop' into flit

a025d01

chrisjsewell requested a review from csadorf January 17, 2022 09:53

csadorf previously approved these changes Jan 17, 2022

View reviewed changes

sphuber reviewed Jan 17, 2022

View reviewed changes

remove arg from check_release_tag.py

a2f57cd

chrisjsewell dismissed csadorf’s stale review via a2f57cd January 17, 2022 13:17

chrisjsewell requested a review from sphuber January 17, 2022 13:24

csadorf approved these changes Jan 17, 2022

View reviewed changes

sphuber approved these changes Jan 17, 2022

View reviewed changes

chrisjsewell merged commit d9f8c85 into aiidateam:develop Jan 17, 2022

chrisjsewell deleted the flit branch January 17, 2022 14:04

chrisjsewell mentioned this pull request Jan 18, 2022

👌 Refresh template plugin aiidateam/aiida-plugin-cutter#88

Merged

chrisjsewell mentioned this pull request Feb 22, 2022

🔧 MAINTAIN: Move to flit for PEP 621 package builds jupyter-book/jupyter-book#1645

Merged

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

JPchico mentioned this pull request Jun 23, 2022

Change to build using pyproject.toml UppASD/aiida-uppasd#4

Closed

danielhollas mentioned this pull request Jul 27, 2022

Switch setup.json to setup.cfg ezpzbz/aiida-orca#38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔧 Move to flit for PEP 621 compliant package build #5312

🔧 Move to flit for PEP 621 compliant package build #5312

chrisjsewell commented Jan 15, 2022 •

edited

Loading

codecov bot commented Jan 15, 2022 •

edited

Loading

chrisjsewell commented Jan 16, 2022 •

edited

Loading

chrisjsewell commented Jan 16, 2022 •

edited

Loading

csadorf left a comment •

edited by chrisjsewell

Loading

chrisjsewell commented Jan 17, 2022 •

edited

Loading

csadorf commented Jan 17, 2022

sphuber left a comment

sphuber Jan 17, 2022

chrisjsewell Jan 17, 2022

chrisjsewell Jan 17, 2022 •

edited

Loading

sphuber Jan 17, 2022

csadorf Jan 17, 2022

sphuber Jan 17, 2022

chrisjsewell Jan 17, 2022

sphuber Jan 17, 2022

chrisjsewell Jan 17, 2022 •

edited

Loading

sphuber Jan 17, 2022

chrisjsewell Jan 17, 2022

sphuber Jan 17, 2022

chrisjsewell Jan 17, 2022 •

edited

Loading

chrisjsewell commented Jan 17, 2022



		def get_version_from_module(content: str) -> str:
		"""Get the __version__ value from a module."""

🔧 Move to flit for PEP 621 compliant package build #5312

🔧 Move to flit for PEP 621 compliant package build #5312

Conversation

chrisjsewell commented Jan 15, 2022 • edited Loading

codecov bot commented Jan 15, 2022 • edited Loading

Codecov Report

chrisjsewell commented Jan 16, 2022 • edited Loading

chrisjsewell commented Jan 16, 2022 • edited Loading

csadorf left a comment • edited by chrisjsewell Loading

Choose a reason for hiding this comment

chrisjsewell commented Jan 17, 2022 • edited Loading

csadorf commented Jan 17, 2022

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

chrisjsewell commented Jan 17, 2022

chrisjsewell commented Jan 15, 2022 •

edited

Loading

codecov bot commented Jan 15, 2022 •

edited

Loading

chrisjsewell commented Jan 16, 2022 •

edited

Loading

chrisjsewell commented Jan 16, 2022 •

edited

Loading

csadorf left a comment •

edited by chrisjsewell

Loading

chrisjsewell commented Jan 17, 2022 •

edited

Loading

chrisjsewell Jan 17, 2022 •

edited

Loading

chrisjsewell Jan 17, 2022 •

edited

Loading

chrisjsewell Jan 17, 2022 •

edited

Loading