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

Add support for pyproject.toml #3301

Merged

Conversation

agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Aug 30, 2023

Description

There are a few nice-to-haves and caveats to this migration to pyproject.toml:

  1. The setup.py file cannot be removed completely due to its flexibility with custom wheel build processes and steps to build extension modules, so this is more of an addition/conjugation with setuptools.build_meta rather than a complete migration—the latter is not possible without the inclusion of a new build-backend such as meson-build or scikit-build-core. Note: the scikit-build-core's support for editable installations is not mature and might break things, and scikit-build is the predecessor to it with limited support for editable installations, which is a deal-breaker.
  2. Project metadata can now be included inside pyproject.toml, which simplifies the setuptools.setup() command. The CMakeBuild.py script is directly included inside setup.py to expose the linking of the IDAKLU extension and usage of the CMake arguments.

A list of tasks:

  • Editable wheels on Windows, Linux, and macOS are working on GitHub Actions, and on ARM-based macOS machines locally. I will test them on a Windows system locally as well.
  • pip is able to build non-editable wheels
  • Both isolated and non-isolated builds work
  • Unit and integration tests are passing on all platforms
  • Doctests, example notebooks, and example scripts are passing
  • cibuildwheel jobs pass for building reproducible wheels for all platforms

Fixes #3049 and supersedes #3053

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@agriyakhetarpal
Copy link
Member Author

So far unit and integration tests passed for both Windows and macOS but cibuildwheel failed on macOS and passed on Windows. With nox in the CI we run on a global installation of PyBaMM and not in a virtual environment, so we technically don't need to build without isolation. I am unsure how Read the Docs succeeds—we install the [docs] and [all] extras on an Ubuntu distribution, so it should have failed there like it did for the unit tests and other test groups on Ubuntu...

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d5dae10) 99.58% compared to head (f1fd05f) 99.58%.
Report is 7 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3301   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        20119    20119           
========================================
  Hits         20036    20036           
  Misses          83       83           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pyproject.toml Outdated Show resolved Hide resolved
@Saransh-cpp
Copy link
Member

I thought we were merging develop into main and adding a git tag on a commit on that branch to create a release.

The v23.9 branch will be merged into main to create the final release. Merging PRs in develop will not affect the final release in any way. Any bug/security fix that is required in the final release should be merged into develop (like other PRs) and then cherry-picked to the release branch (v23.9).

Of course, building wheels is not a security issue or a bug that needs fixing but rather a packaging feature

Yes, that sounds like a feature. It would be better to include that in the next release!

@agriyakhetarpal
Copy link
Member Author

Makes sense, thanks for the explanation!

Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

I've tested wheels for amd64 Linux (WSL) and amd64 (Windows), they're being installed nicely. I've verified the solvers too by pybamm.have_idaklu() and all and also ran unit tests locally.
Overall tests are running as expected & those with solver dependencies are being skipped.

@Saransh-cpp
Copy link
Member

@agriyakhetarpal could you merge develop? I was waiting for the release, but now that it is out, we should merge this as soon as possible

@agriyakhetarpal
Copy link
Member Author

@agriyakhetarpal could you merge develop? I was waiting for the release, but now that it is out, we should merge this as soon as possible

Done. Since I am targeting some installation issues, improvements, and documentation in the coming time—it should be fine to merge once @kratman can review (a gentle reminder!)—I can add a minimal test to check for the IDAKLU solver during the cibuildwheel steps, but I think it would be better to do that in a follow-up PR since this is already very big.

@agriyakhetarpal
Copy link
Member Author

I have disabled the wheel building on PRs like it always was and have triggered a final run on my fork, here is the link: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/6935629522

Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

Minor fix for the version.

Nox installs/text/examples work on my Apple M2. I will download the wheels and test them as well.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
@agriyakhetarpal
Copy link
Member Author

I will download the wheels and test them as well.

@arjxn-py has previously tested ans confirmed on WSL and Windows (amd64). We don't have arm64 wheels yet, so another test on a native Linux machine should be great and would suffice for the most part

@kratman
Copy link
Contributor

kratman commented Nov 20, 2023

I will download the wheels and test them as well.

@arjxn-py has previously tested ans confirmed on WSL and Windows (amd64). We don't have arm64 wheels yet, so another test on a native Linux machine should be great and would suffice for the most part

I am testing Linux Mint with x86_64 right now. Should be able to let you know if it works in a couple minutes

@kratman
Copy link
Contributor

kratman commented Nov 20, 2023

Works on my Linux machine

@Saransh-cpp
Copy link
Member

Merging this, I hope everything works fine!

@Saransh-cpp Saransh-cpp merged commit a463246 into pybamm-team:develop Nov 25, 2023
32 of 35 checks passed
@agriyakhetarpal agriyakhetarpal deleted the pyproject-toml-migration branch November 25, 2023 14:17
@agriyakhetarpal
Copy link
Member Author

So glad to see the purple UI elements now, thanks a ton to everyone who helped me out!

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.

Migrate or conjugate pyproject.toml with setup.py
6 participants