csyhuang / hn2016_falwa

Python library for computing Finite-Amplitude Local Wave Activity Diagnostics from climate data.
https://hn2016-falwa.readthedocs.io/en/latest/
MIT License
32 stars 16 forks source link

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

Closed chpolste closed 1 month ago

chpolste commented 2 months ago

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:

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:

codecov-commenter commented 2 months ago

:warning: 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 :white_check_mark:

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

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

csyhuang commented 2 months ago

@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 commented 1 month ago

@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 commented 1 month ago

@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