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

src-layout, pyproject.toml and meson-python build backend #126

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

chpolste
Copy link
Collaborator

@chpolste chpolste commented Aug 4, 2024

A new pull request to follow up on #73, which was a bit messy after multiple updates to master.

The proposed changes here are quite substantial in terms of how the package is built and likely breaks some current development practices. @csyhuang: I'm not sure if this is acceptable. There might be a less disruptive way to make this work, but I couldn't figure it out. The commit on this branch so far:

  • Replaces the setup.py with a pyproject.toml file.
  • Moves the falwa package into a src subdirectory. Because meson only supports out-of-source builds, it does not generate the compiled Fortran modules in the falwa directory and the local package from the falwa folder in the repository cannot be imported. This then breaks the current test workflow, because when pytest is run from the root of the repository, it defaults to importing the local package rather than the installed package and fails. With a src directory in the middle, the local development files are isolated and tests are run on the installed package. Many packages out there don't seem to need this, but I couldn't figure out a way to make everything work without.
  • After the removal of numpy.disutils for Python versions >= 3.12, compilation of the Fortran extensions and packaging is now carried out with meson.
  • To simplify the handling in meson, the files interpolate_fields_dirinv.f90 and compute_qgpv_dirinv in src/falwa/f90_modules have been renamed with the suffix _direct_inv to match the name of the contained module.
  • The automated tests now include Python 3.12, but no longer 3.9.

I currently don't know how this impacts package deployment via PyPI and conda. I'm guessing it won't work exactly the same as before with the setup.py and numpy.distutils. The package deployment instructions in notes/developer certainly need to be updated. Same with the install instructions in the top-level README.

Some documentation/examples on meson I found useful during the conversion:

All in one commit as these changes are highly interconnected.

- Replace setup.py with pyproject.toml.
- Moved the falwa package into a src subdirectory to isolate the local
  development files and make sure that tests are run on the installed package.
  Necessary due to the out-of-source build behavior of meson.
- After the removal of numpy.disutils for Python versions >= 3.12, compilation
  of the Fortran extensions and packaging is now carried out with meson.
- To simplify the handling in meson, the files interpolate_fields_dirinv.f90
  and compute_qgpv_dirinv in src/falwa/f90_modules have been renamed with the
  suffix _direct_inv to match the name of the contained module.

The pyproject.toml uses the meson build backend rather than setuptools and
numpy.distutils. The setup.py file has been deleted.

The automated tests on GitHub have been upgraded to include Python 3.12, which
can now be supported, and exclude 3.9.

TODO: package deployment instructions in notes/developer need to be adapted.
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.09%. Comparing base (2c35086) to head (6295a7d).
Report is 4 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage   66.71%   57.09%   -9.63%     
==========================================
  Files          13       11       -2     
  Lines        1274     1487     +213     
==========================================
- Hits          850      849       -1     
- Misses        424      638     +214     

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

@csyhuang
Copy link
Owner

csyhuang commented Aug 5, 2024

@chpolste Hi Christopher,

Great thanks for figuring out a solution and submitting this pull request! That's a lot of work!

I am not aware of any other active developers who would modify the source code, so changing the installation mode is fine. We just have to state that explicitly in documentation and let the users know.

I will test the packaging on testPyPI and try installing our package in MDTF this week to see if any adjustments are needed. I will also update the package paper according to this new package structure - probably I'll send you the first draft later this week.

Thank you so much and I'll get back to you soon! 😊

Best,

Clare

@csyhuang
Copy link
Owner

csyhuang commented Aug 7, 2024

@chpolste Hi Christopher,

I tested the installation on Mac and Linux machines - both worked well 😁

For installation via PyPI, I tried uploading onto test.pypi to test installation using pip, but there were some indexing issue (on test.pypi side) - it looks like I can only test pip install if I deploy for real on PyPI. Nonetheless, I tried downloading the compiled .tar.gz file from test.pypi and then install with python3 -m pip ....tar.gz and it worked, so I think we are good to go. 👍

Another way to do it is to specify in the MDTF environment file to install from GitHub directly (Tested. It worked):

name: _MDTF_python3_base
channels:
- conda-forge # NOTE: critical to give highest priority to conda-forge
dependencies:
- python=3.12
...
- pip=23.3.1
- pip:
    - git+https://github.com/csyhuang/hn2016_falwa.git@meson-build-test-pypi

(MDTF already have a bunch of packages from GitHub under pip, so adding one more should be fine)

Given there's a significant change in repo structure and the installation mechanism, let's instead make a major release (v2.0.0) such that users are aware of the substantial change?

Let me know what you think 😊 Once we make the release, I'll email the MDTF team to see if they can test the installation on their side. Thanks again for figuring out a clean and timely solution for the F2PY modules!

Cheers,

Clare

@csyhuang csyhuang merged commit edd6282 into master Aug 16, 2024
3 checks passed
@csyhuang csyhuang deleted the meson-build branch August 16, 2024 20:22
@csyhuang
Copy link
Owner

@chpolste Hi Christopher, Jess from MDTF informed me that this new version of falwa passed their test, and they have included that in their base environment: 😊

https://github.com/NOAA-GFDL/MDTF-diagnostics/blob/47e9cf7142db6887a6190039ffeca9d28ee114d7/src/conda/env_python3_base.yml#L43

We are close to the finishing line! After fixing the catalog #119 and adding back description, our POD will be completed! I'll strive for finishing that in the coming two weeks!

Thanks a lot for figuring out the meson-build solution!!

Cheers,

Clare

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.

3 participants