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.

Sign up for GitHub

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

build: Switch to using Hatchling as build backend #2095

Merged
merged 36 commits into from
Jan 20, 2023
Merged

build: Switch to using Hatchling as build backend #2095

Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
7ab62c1
Generate hatch init from hatch CLI
matthewfeickert Jan 18, 2023
eda4b1e
Add FIXME for recursive packages
matthewfeickert Jan 18, 2023
d79fe48
Migrate comments for dependencies
matthewfeickert Jan 18, 2023
1377f19
Use single quotes for readability
matthewfeickert Jan 18, 2023
c1cc7cf
Change sort
matthewfeickert Jan 18, 2023
3395c50
Remove setup.cfg in favor of pyproject.toml
matthewfeickert Jan 18, 2023
7e4c78c
Add flake8
matthewfeickert Jan 18, 2023
e5b3e40
Remove setuptools_scm
matthewfeickert Jan 18, 2023
fab3baf
Try to correct license metadata
matthewfeickert Jan 18, 2023
38cda4a
Add include targets for sdist
matthewfeickert Jan 18, 2023
4b09eec
Use SPDX short identifier for license
matthewfeickert Jan 18, 2023
174da49
Add excludes
matthewfeickert Jan 18, 2023
10668ec
Set lower bound for hatchling
matthewfeickert Jan 18, 2023
3cfce28
can't ingore .gitignore
matthewfeickert Jan 18, 2023
d73b955
Organize backends
matthewfeickert Jan 18, 2023
a0ba25f
Place some reasonable lower bounds on matplotlib and requests
matthewfeickert Jan 18, 2023
da9f3e8
Use pip 21.2: Recursive Dependencies
matthewfeickert Jan 19, 2023
898dcf1
Try to setup a test hatch env
matthewfeickert Jan 19, 2023
ebb29a9
Try to add install and pytest script
matthewfeickert Jan 19, 2023
eb23550
Use giordon's fix
matthewfeickert Jan 19, 2023
2e6c999
unbreak scripts
matthewfeickert Jan 19, 2023
635e9c4
Add extras
matthewfeickert Jan 19, 2023
0cd6311
Remove setup.py
matthewfeickert Jan 19, 2023
8df4a84
Remove MANIFEST.in
matthewfeickert Jan 19, 2023
1715356
Move FIXME
matthewfeickert Jan 19, 2023
02263a8
Fix .flake8 config
matthewfeickert Jan 19, 2023
9687fae
Apply typo fix and hatch best practice fix from Angus
matthewfeickert Jan 19, 2023
1f3750c
docs: Remove mention of setup.cfg from release checklist
matthewfeickert Jan 19, 2023
ae5b0d7
Add visual spacers
matthewfeickert Jan 19, 2023
68c180c
temporarily drop the user of hatch envs
matthewfeickert Jan 19, 2023
1bf9c0e
match old setuptools_scm system of not including the commit hash
matthewfeickert Jan 19, 2023
4a4da3e
Remove tool.hatch.build.targets.wheel
matthewfeickert Jan 19, 2023
9ca5b12
Use only-include to avoid broken schemas
matthewfeickert Jan 19, 2023
f3ebf0e
clarify it is only-include
matthewfeickert Jan 19, 2023
6770b70
Revert "Remove tool.hatch.build.targets.wheel"
matthewfeickert Jan 20, 2023
d1fe796
Switch positions of exclude and only-include for readability
matthewfeickert Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[flake8]
kratsg marked this conversation as resolved.
Show resolved Hide resolved
# E203: whitespace before ':'
# E402: module level import not at top of file
# E501: line too long
extend-ignore = E203, E402, E501
max-line-length = 88
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/~release-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ about: Checklist for core developers to complete as part of making a release
* [ ] Verify that a Binder has properly built for the new release.
* [ ] Watch for a GitHub notification that there is an automatic PR to the [Conda-forge feedstock](https://github.com/conda-forge/pyhf-feedstock). This may take multiple hours to happen. If there are any changes needed to the Conda-forge release make them **from a personal account** and not from an organization account to have workflows properly trigger.
- [ ] Check if any requirements need to be updated by commenting "@conda-grayskull show requirements" on the PR.
- [ ] Verify the requirements in the [Conda-forge feedstock](https://github.com/conda-forge/pyhf-feedstock) recipe `meta.yaml` match those in `setup.cfg` and `pyproject.toml`.
- [ ] Verify the requirements in the [Conda-forge feedstock](https://github.com/conda-forge/pyhf-feedstock) recipe `meta.yaml` match those in `pyproject.toml`.

## After Release

Expand Down
10 changes: 3 additions & 7 deletions .github/workflows/publish-package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,12 @@ jobs:
with:
python-version: '3.10'

- name: Install python-build, check-manifest, and twine
- name: Install python-build and twine
run: |
python -m pip install --upgrade pip setuptools wheel
python -m pip install build check-manifest twine
python -m pip install --upgrade pip
python -m pip install build twine
python -m pip list

- name: Check MANIFEST
run: |
check-manifest

- name: Build a wheel and a sdist
run: |
PYTHONWARNINGS=error,default::DeprecationWarning python -m build .
Expand Down
13 changes: 0 additions & 13 deletions MANIFEST.in
View file
Open in desktop

This file was deleted.

162 changes: 139 additions & 23 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,146 @@
[build-system]
# Minimum requirements for the build system to execute.
requires = ["setuptools>=61.0.0", "setuptools_scm>=7.0.1"]
build-backend = "setuptools.build_meta"
requires = ["hatchling>=1.0.0", "hatch-vcs"]
build-backend = "hatchling.build"

[tool.setuptools_scm]
write_to = "src/pyhf/_version.py"
[project]
name = "pyhf"
dynamic = ["version"]
description = "pure-Python HistFactory implementation with tensors and autodiff"
readme = "README.rst"
matthewfeickert marked this conversation as resolved.
Show resolved Hide resolved
license = { text = "Apache-2.0" } # SPDX short identifier
requires-python = ">=3.8"
authors = [
{ name = "Lukas Heinrich", email = "lukas.heinrich@cern.ch" },
{ name = "Matthew Feickert", email = "matthew.feickert@cern.ch" },
{ name = "Giordon Stark", email = "gstark@cern.ch" },
matthewfeickert marked this conversation as resolved.
Show resolved Hide resolved
]
maintainers = [ {name = "The Scikit-HEP admins", email = "scikit-hep-admins@googlegroups.com"} ]
keywords = [
"fitting",
"jax",
"numpy",
"physics",
"pytorch",
"scipy",
"tensorflow",
]
matthewfeickert marked this conversation as resolved.
Show resolved Hide resolved
classifiers = [
"Development Status :: 4 - Beta",
"Intended Audience :: Science/Research",
"License :: OSI Approved :: Apache Software License",
"Programming Language :: Python :: 3",
"Programming Language :: Python :: 3 :: Only",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: Implementation :: CPython",
"Topic :: Scientific/Engineering",
"Topic :: Scientific/Engineering :: Physics",
]
dependencies = [
"click>=8.0.0", # for console scripts
"importlib_resources>=1.4.0; python_version < '3.9'", # for resources in schema
"jsonpatch>=1.15",
"jsonschema>=4.15.0", # for utils
"pyyaml>=5.1", # for parsing CLI equal-delimited options
"scipy>=1.5.1", # requires numpy, which is required by pyhf and tensorflow
"tqdm>=4.56.0", # for readxml
"numpy", # compatible versions controlled through scipy
]

[project.scripts]
pyhf = "pyhf.cli:cli"

[project.urls]
Documentation = "https://pyhf.readthedocs.io/"
Homepage = "https://github.com/scikit-hep/pyhf"
matthewfeickert marked this conversation as resolved.
Show resolved Hide resolved
"Issue Tracker" = "https://github.com/scikit-hep/pyhf/issues"
"Release Notes" = "https://pyhf.readthedocs.io/en/stable/release-notes.html"
"Source Code" = "https://github.com/scikit-hep/pyhf"

[project.optional-dependencies]
shellcomplete = ["click_completion"]
tensorflow = [
"tensorflow>=2.7.0", # c.f. PR #1962
"tensorflow-probability>=0.11.0", # c.f. PR #1657
]
torch = ["torch>=1.10.0"] # c.f. PR #1657
jax = [
"jax>=0.4.1", # c.f. PR #2079
"jaxlib>=0.4.1", # c.f. PR #2079
]
xmlio = ["uproot>=4.1.1"] # c.f. PR #1567
minuit = ["iminuit>=2.7.0"] # c.f. PR #1895
contrib = [
"matplotlib>=3.0.0",
"requests>=2.22.0",
]
backends = ["pyhf[tensorflow,torch,jax,minuit]"]
all = ["pyhf[backends,xmlio,contrib,shellcomplete]"]

# Developer extras
test = [
"pyhf[all]",
"scikit-hep-testdata>=0.4.11",
"pytest>=6.0",
"coverage[toml]>=6.0.0",
"pytest-mock",
"requests-mock>=1.9.0",
"pytest-benchmark[histogram]",
"pytest-console-scripts",
"pytest-mpl",
"pydocstyle",
"papermill~=2.3.4",
"scrapbook~=0.5.0",
"jupyter",
"graphviz",
"pytest-socket>=0.2.0", # c.f. PR #1917
]
docs = [
"pyhf[xmlio,contrib]",
"sphinx>=5.1.1", # c.f. https://github.com/scikit-hep/pyhf/pull/1926
"sphinxcontrib-bibtex~=2.1",
"sphinx-click",
"sphinx_rtd_theme",
"nbsphinx!=0.8.8", # c.f. https://github.com/spatialaudio/nbsphinx/issues/620
"ipywidgets",
"sphinx-issues",
"sphinx-copybutton>=0.3.2",
"sphinx-togglebutton>=0.3.0",
"ipython!=8.7.0", # c.f. https://github.com/scikit-hep/pyhf/pull/2068
]
develop = [
"pyhf[test,docs]",
"tbump>=6.7.0",
"pre-commit",
"nox",
"codemetapy>=2.3.0",
]

[tool.hatch.version]
source = "vcs"

matthewfeickert marked this conversation as resolved.
Show resolved Hide resolved
[tool.hatch.version.raw-options]
local_scheme = "no-local-version"

[tool.hatch.build.hooks.vcs]
version-file = "src/pyhf/_version.py"

[tool.hatch.build.targets.sdist]
exclude = [
"/src/conftest.py"
]
# Needed to properly include src/pyhf/schemas
# c.f. https://github.com/pypa/hatch/pull/299
only-include = [
Copy link
Member Author

Choose a reason for hiding this comment

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

Okay so with the addition of only-include the schemas dir is getting properly picked up by the sdist.

I'm a bit confused though given the docs https://hatch.pypa.io/latest/config/build/#packages

The packages option is semantically equivalent to only-include (which takes precedence) except that the shipped path will be collapsed to only include the final component.

So for example, if you want to ship a package foo that is stored in a directory src you would do:

[tool.hatch.build.targets.wheel]
packages = ["src/foo"]

what "final component" means. Though there seems to be no difference in behavior between including or not including

[tool.hatch.build.targets.wheel]
packages = ["src/pyhf"]

Copy link
Contributor

@agoose77 agoose77 Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

packages is like only-include combined with sources, where sources would be sources = ["src"] for this example. It means that packages = ["src/this/that/foo/bar"] would behave like

only-include = ["src/this/that/foo/bar"]
sources = ["src/this/that/foo"]

i.e. the final path component (bar) is the name of the package as included in the final wheel (because it's taken relative to sources).

Copy link
Member Author

Choose a reason for hiding this comment

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

$ rm -rf dist && python -m build . && python -m tarfile --list dist/pyhf*.tar.gz | sed 's|[^/]*/||' > /tmp/sdist-without.txt && python -m zipfile --list dist/pyhf*.whl | sed 's|[^/]*/||' > /tmp/wheel-without.txt
$ git revert 4a4da3e0
$ rm -rf dist && python -m build . && python -m tarfile --list dist/pyhf*.tar.gz | sed 's|[^/]*/||' > /tmp/sdist-with.txt && python -m zipfile --list dist/pyhf*.whl | sed 's|[^/]*/||' > /tmp/wheel-with.txt
$ diff /tmp/sdist-without.txt /tmp/sdist-with.txt  # nothing
$ diff /tmp/wheel-without.txt /tmp/wheel-with.txt  # nothing

Copy link
Member Author

@matthewfeickert matthewfeickert Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i.e. the final path component (bar) is the name of the package as included in the final wheel (because it's taken relative to sources).

So is the reason that removing

[tool.hatch.build.targets.wheel]
packages = ["src/pyhf"]

didn't have any effect is because we're using only-include for [tool.hatch.build.targets.sdist]

[tool.hatch.build.targets.sdist]
exclude = [
    "/src/conftest.py"
]
# only-include needed to properly include src/pyhf/schemas
# c.f. https://github.com/pypa/hatch/pull/299
only-include = [
    "/src",
    "/LICENSE",
    "/README.rst",
    "/pyproject.toml",
    "/AUTHORS",
    "/CITATION.cff"
]

and we only have one package directory in src/?

Copy link
Contributor

@agoose77 agoose77 Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK, the original issue that we were fixing with only-include has been fixed (I think) by copying the schemas into the docs at docs time: 81a429e

So we don't need this explicitly. I found that reverting 9ca5b12 didn't leave the schemas out of the sdist or wheel for me. If you revert 9ca5b12 and keep 4a4da3e, what do you see?

i.e., I think the default Hatch build configuration should pick everything up. But, if you want to use e.g. only-include, you can for other reasons

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just being verbose here for ease. This might be your understanding too, but I want to be sure we are all on the same page! :)

Python build --wheel is doing the recommended thing of sdist → wheel. Hatch's default file inclusion logic always picks up the pyhf/schemas directory, but the symlinked schemas in docs/_extras/ cause this to fail. Eliminating these from the SDist (manually, with only-include et al.) means that the wheel builder then succeeds with default inclusion logic, assuming that indeed we go sdist → wheel.

Meanwhile, hatch build --target wheel doesn't do sdist → wheel, and therefore the wheel build suffers the same problem as the sdist; the schemas are found, but encountered under the wrong (excluded) path (docs/_extras)

So we definitely need an only-include like directive in both targets.sdist and targets.wheel whilst we have these duplicate schemas.

Copy link
Member Author

@matthewfeickert matthewfeickert Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Python build --wheel is doing the recommended thing of sdist → wheel.

I don't think that's exactly correct. From python -m build --help:

    By default, a source distribution (sdist) is built from {srcdir}
    and a binary distribution (wheel) is built from the sdist.
    This is recommended as it will ensure the sdist can be used
    to build wheels.

    Pass -s/--sdist and/or -w/--wheel to build a specific distribution.
    If you do this, the default behavior will be disabled, and all
    artifacts will be built from {srcdir} (even if you combine
    -w/--wheel with -s/--sdist, the wheel will be built from {srcdir}).

so my python -m build --wheel . is building from the srcdir, and not from sdist (the default behavior).

I agree on the rest though, except that I think from the above targets.wheel doesn't need the only-include like directive but that I want to leave it in regardless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gah, then "Python build --wheel is not doing the recommended thing of sdist → wheel" 😆

Admission time ... I discovered my environment didn't fully upgrade; I was evaluating hatch behaviour in-part based upon CLI exploration. I upgraded hatchling to the newest version explicitly, and the different results that I was seeing evaporated. I had been confused that we seemed to be drawing different conclusions.

Now Ofek's points make more sense — for the SDist, we need to curtail the default search for the above reasons, but the wheel builder works fine due to the comment from Ofek that you linked!

So my view is different now; we don't need that setting, and we only added it because it fixes an annoying build problem. As long as hatchling's PEP 517 hook produces a good wheel when used without an intermediate sdist (which it does), it should be fine to remove it. So, your call, but I think you can safely remove it!

Sorry for the wild goose chase. I had thought I was being helpful, but with the above context I can see it was actually just confusing things!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing to apologize about, you have been beyond helpful. :)

Now Ofek's points make more sense — for the SDist, we need to curtail the default search for the above reasons, but the wheel builder works fine due to the comment from Ofek that you linked!

✔️ 👍 We are in agreement on understanding now!

As long as hatchling's PEP 517 hook produces a good wheel when used without an intermediate sdist (which it does), it should be fine to remove it. So, your call, but I think you can safely remove it!

We can remove it but keeping it in skips all of

https://github.com/pypa/hatch/blob/hatchling-v1.12.2/backend/src/hatchling/builders/wheel.py#L172-L200

which while I'm sure is only a few microseconds, seems like a "why not just leave in 2 lines?" question to me now.

Copy link
Contributor

@ofek ofek Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hatch currently does not build the wheel from the source distribution. I plan to do that but I don't have time currently. It would actually be useful if you did open an issue so I can track it

"/src",
"/LICENSE",
"/README.rst",
"/pyproject.toml",
"/AUTHORS",
"/CITATION.cff"
]

[tool.black]
line-length = 88
target-version = ['py38', 'py39', 'py310']
Expand All @@ -21,24 +155,6 @@ exclude = '''
)/
'''

[tool.check-manifest]
ignore = [
'docs*',
'validation*',
'examples*',
'tests*',
'docker*',
'binder*',
'.*',
'pyproject.toml',
'pytest.ini',
'codecov.yml',
'codemeta.json',
'CODE_OF_CONDUCT.md',
'CONTRIBUTING.md',
'AUTHORS',
]

[tool.pytest.ini_options]
minversion = "6.0"
xfail_strict = true
Expand Down
58 changes: 0 additions & 58 deletions setup.cfg
View file
Open in desktop

This file was deleted.

87 changes: 0 additions & 87 deletions setup.py
View file
Open in desktop

This file was deleted.