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

Start transitioning to pyproject.toml #23829

Closed
wants to merge 1 commit into from

Conversation

oscargus
Copy link
Contributor

@oscargus oscargus commented Sep 8, 2022

PR Summary

Related to #23815

Issues:

  FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpheklt6mf.build-lib/matplotlib/mpl-data/matplotlibrc'


              If you are seeing this warning it is very likely that a setuptools
              plugin or customization overrides the `build_py` command, without
              tacking into consideration how editable installs run build steps
              starting from v64.0.0.

              Plugin authors and developers relying on custom build steps are encouraged
              to update their `build_py` implementation considering the information in
              https://setuptools.pypa.io/en/latest/userguide/extension.html
              about editable installs.

              For the time being `setuptools` will silence this error and ignore
              the faulty command, but this behaviour will change in future versions.

Lots of:

  /tmp/pip-build-env-7a9fvqdh/overlay/lib/python3.10/site-packages/setuptools/command/build_py.py:202: SetuptoolsDeprecationWarning:     Installing 'matplotlib.tests.baseline_images.test_polar' as data is deprecated, please list it in `packages`.
      !!


      ############################
      # Package would be ignored #
      ############################
      Python recognizes 'matplotlib.tests.baseline_images.test_polar' as an importable package,
      but it is not listed in the `packages` configuration of setuptools.

      'matplotlib.tests.baseline_images.test_polar' has been automatically added to the distribution only
      because it may contain data files, but this behavior is likely to change
      in future versions of setuptools (and therefore is considered deprecated).

      Please make sure that 'matplotlib.tests.baseline_images.test_polar' is included as a package by using
      the `packages` configuration field or the proper discovery methods
      (for example by using `find_namespace_packages(...)`/`find_namespace:`
      instead of `find_packages(...)`/`find:`).

      You can read more about "package discovery" and "data files" on setuptools
      documentation page.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus oscargus force-pushed the pyproject branch 2 times, most recently from 3fe5d58 to 9b87d69 Compare September 8, 2022 19:44
@oscargus
Copy link
Contributor Author

oscargus commented Sep 8, 2022

More issues:

Not building on min-version

RuntimeError: module compiled against API version 0x10 but this version of numpy is 0xd

No C coverage:

Reading tracefile coverage.info
Extracted 0 files
Writing data to coverage.info
Summary coverage rate:
  lines......: no data found
  functions..: no data found
  branches...: no data found
lcov: ERROR: no valid records found in tracefile coverage.info
Reading tracefile coverage.info
Error: Process completed with exit code 2

pyproject.toml Outdated
]

[build-system]
requires = ["setuptools>=45", "setuptools_scm[toml]>=7", "wheel", "certifi>=2020.06.20", "numpy>=1.19"]
Copy link
Member

Choose a reason for hiding this comment

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

This should be oldest-supported-numpy, see https://github.com/scipy/oldest-supported-numpy

The problem is that if you put a floor here, then when the background env gets made it will pull the newest version of numpy it can find. Numpy promises that things complied with old numpy will work with new numpy at runmtime, but they do not promise that things compiled with new numpy will work with old numpy at runtime.

Suggested change
requires = ["setuptools>=45", "setuptools_scm[toml]>=7", "wheel", "certifi>=2020.06.20", "numpy>=1.19"]
requires = ["setuptools>=45", "setuptools_scm[toml]>=7", "wheel", "certifi>=2020.06.20", "oldest-supported-numpy"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it seems like the following happens:

  xarray 2022.6.0 requires numpy>=1.19, but you have numpy 1.17.3 which is incompatible.
  pandas 1.4.4 requires numpy>=1.18.5; platform_machine != "aarch64" and platform_machine != "arm64" and python_version < "3.10", but you have numpy 1.17.3 which is incompatible.

@tacaswell tacaswell added this to the v3.7.0 milestone Sep 12, 2022
@oscargus oscargus force-pushed the pyproject branch 2 times, most recently from 60890cd to 8ada724 Compare September 15, 2022 20:01
@ianthomas23
Copy link
Member

In the long run, when all the C/C++ extensions are wrapped using pybind11 we won't need any numpy in the [build-system] requires section. To make headway here I think it needs to be "numpy==1.19".

We might have to be careful with the minver test in CI. Because of the build isolation mpl will build against the build-system requires rather than the preinstalled minver packages. So if we don't modify the CI then the minimum-versions test will test but not build against the minimum versions whereas now it both builds and tests against them. This might be fine; if not we will probably need to pip install --no-build-isolation -ve . (or similar) for the minimum versions test only.

@oscargus
Copy link
Contributor Author

oscargus commented Oct 4, 2022

New error in some builds:

          File "numpy/core/setup.py", line 655, in get_mathlib_info
            raise RuntimeError("Broken toolchain: cannot link a simple C program")
        RuntimeError: Broken toolchain: cannot link a simple C program

@ianthomas23
Copy link
Member

It is probably worth rebasing this on main to see where we've got to.

pyproject.toml Outdated Show resolved Hide resolved
@oscargus
Copy link
Contributor Author

Looks surprisingly promising. Wouldn't trust it even if everything happens to pass though (hasn't happened yet).

For example, I do not really know how to deal with the namespace package thing (not what it actually is).

"pillow>=6.2.0",
"pyparsing>=2.2.1",
"python-dateutil>=2.7",
"setuptools_scm>=7.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be conditional, but I have not found any info how to do this dynamically.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is needed at all as it is in the [build-system] requires section. It is needed by anyone who uses --no-build-isolation in their pip install ., but in that situation they will need to have preinstalled all of the [build-system] requires packages to proceed.

Although I might be missing some other use of setuptools_scm in testing or somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added as someone had problems running tests in a particular setup(?).

Without it, this breaks in the tests:

if ((root / ".matplotlib-repo").exists()
and (root / ".git").exists()
and not (root / ".git/shallow").exists()):
import setuptools_scm
return setuptools_scm.get_version(
root=root,
version_scheme="release-branch-semver",
local_scheme="node-and-date",
fallback_version=_version.version,
)

although it indeed is not really required for a user install.

Copy link
Member

Choose a reason for hiding this comment

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

I see now. So it shouldn't be in the dependencies section of the pyproject.toml as then wheel users will be forced to have it which means it should be in one of the requirements files instead, perhaps requirements/testing/all.txt?

Eventually I would expect the requirements files to be replaced with [project.optional-dependencies] sections in the pyproject.toml, but that is not necessary yet.

@oscargus
Copy link
Contributor Author

Wheels are 37 MB. Probably as the test images are included...

setup.py Outdated Show resolved Hide resolved
@oscargus oscargus force-pushed the pyproject branch 2 times, most recently from e921399 to 3d8c9c8 Compare October 19, 2022 12:54
@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 15, 2022
@tacaswell
Copy link
Member

Push this to mpl 3.8 to avoid any last-minute packaging issues for mpl 3.7.

@ianthomas23
Copy link
Member

@oscargus I've experimented with this locally and I think I have made some progress. If I add this new section to the pyproject.toml:

[tool.setuptools]
include-package-data = false

then the test images are excluded from the wheel. That might be excluding too much, so if you remove that section (include-package-data is true by default) and add this one instead:

[tool.setuptools.exclude-package-data]
"*" = ["*.png", "*.svg"]

then only the PNGs and SVGs are excluded. That still might be excluding too many files, but it is a start.

@oscargus
Copy link
Contributor Author

Thanks @ianthomas23 ! I've updated it and now I think I know how to check the possible problems as well.

@oscargus oscargus marked this pull request as ready for review February 16, 2023 20:40
@oscargus oscargus force-pushed the pyproject branch 2 times, most recently from cd84117 to 8b2df24 Compare February 16, 2023 20:49
@oscargus oscargus marked this pull request as draft February 16, 2023 20:50
@oscargus oscargus removed the CI: Run cibuildwheel Run wheel building tests on a PR label Feb 16, 2023
@oscargus oscargus added the CI: Run cibuildwheel Run wheel building tests on a PR label Feb 16, 2023
@QuLogic
Copy link
Member

QuLogic commented Jun 12, 2023

We discussed this last week on call; we think it makes sense to start having some kind of dev extra on which we can add dependencies like setuptools-scm. I also think that some of the limitations that arise from purely-declarative will be solved in the Meson port. Since we already need to move to pyproject.toml in order to complete the Meson port, I'm thinking of taking this over and rolling it into that PR.

@oscargus
Copy link
Contributor Author

Feel free to do so! I was trying to learn, and I have learnt, but not sure if my knowledge will be enough to finish it (although the dev-section etc is quite straightforward...).

@ksunden ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023
@QuLogic QuLogic mentioned this pull request Aug 29, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Run cibuildwheel Run wheel building tests on a PR status: needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants