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

Adopt PEP 517 and PEP 621, use raven-hydro package #278

Merged
merged 70 commits into from
May 25, 2023
Merged

Conversation

Zeitsperre
Copy link
Member

@Zeitsperre Zeitsperre commented Apr 19, 2023

Closes #283

Changes

  • Replaced setup.py with new format pyproject.toml using the flit backend.
  • Updated pre-commit hooks to include formatters and checkers for TOML files.
  • Removed the obsolete requirements*.txt and Manifest.in files.
  • Dealt with an import-based error that occurred due to the sequence in which modules are loaded at import (attempting to call ravenpy before it is installed).
  • The build recipes no longer build on each other, so when installing the dev or docs recipe, you must also install the gis recipe. A workaround I've added is to simply list the gis recipes in both for now
  • This PR also updates the GeoServer API calls to at the very least work with the newest GeoPandas (using urllib). A more elegant solution should be looked into (to synchronize owslib / requests / urllib.request calls).
  • This PR leverages the new method of installing the Raven model using https://github.com/Ouranosinc/raven-hydro.

What still needs to be addressed

  • There is now no method of building raven-hydro from PyPI. The current approach was rendered obsolete in pip>=23.1, so we are in the right to retire this method.
  • The CI for the PyPI builds using tox are broken due to the missing raven-hydro binary. There is likely a whole slew of things that must be addressed in tox (removal of tons of now-obsolete workarounds), so don't expect this to work yet.
  • There are documentation files as well as the top-level Makefile that refer to operations using setup.py, and twine. These need to be replaced with their flit-based equivalents. For more information, see: https://flit.pypa.io/en/stable/index.html
    • These issues have been resolved.

Why?

With new standards for tooling and packaging, the setuptools-based compilation approach was very quickly becoming unmaintainable and only supported building on Linux/Unix platforms.

For raven-hydro, scikit-build-core is a setup backend that supports multi-OS builds using CMake (which is a much more approachable crossplatform C/C++ build engine). This renders the setuptools-based installation approach obsolete, as we are now able to install a Python wheel or source distribution specifically for the Raven model that allow users to compile it themselves on pip install (similar to numpy).

@Zeitsperre Zeitsperre added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 19, 2023
@Zeitsperre Zeitsperre requested a review from huard April 19, 2023 20:50
@Zeitsperre Zeitsperre self-assigned this Apr 19, 2023
@Zeitsperre
Copy link
Member Author

The new fiona-related errors are due to changes in geopandas that affect reading files from URLs: geopandas/geopandas#2796

The fix is to simply adjust the way we handle these objects in ravenpy.utilities.geoserver. Benefits include significant read speedups.

Comment on lines 38 to 41
If you wish to install RavenPy and its C-libraries manually, compiling the `Raven` binary for your system,
you can install the entire system directly, placing them in the `bin` folder of your environment.
In order to perform this from Ubuntu/Debian:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Point to the Raven documentation ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just make sure that the Install docs are clear on what needs to be done now, ie. use conda or compile yourself.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@@ -7,7 +7,7 @@
import pytest
import xarray as xr
from filelock import FileLock
from xclim.indicators.land import fit, stats
from xclim.indicators.generic import fit, stats
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this because there is deprecation warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because we have just changed it: Ouranosinc/xclim@0e06e2c

I think we need to pin this to the newest version of xclim as a result!

Copy link
Member Author

Choose a reason for hiding this comment

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

Either that or we can make it backwards-compatible with a try/except block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, let's pin to the newest xclim then.

@Zeitsperre
Copy link
Member Author

I think I've earned this.

its_working_star_wars

@Zeitsperre
Copy link
Member Author

There's an environment inconsistency for conda:

  • The newest raven-hydro is built against the newest libnetcdf/libhdf5/etc.
  • The last build of climpred on conda-forge was in November 2022 (no updates since)

As it stands, the libraries are incompatible:

  - climpred               2.1.6  pyhd8ed1ab_1                 conda-forge                    
  + climpred               2.3.0  pyhd8ed1ab_0                 conda-forge/noarch       Cached
  - raven-hydro            0.2.0  py310h3ce4ad4_0              conda-forge                    
  + raven-hydro              3.6  ha2b8ddb_1                   conda-forge/linux-64     Cached

I think climpred needs to be regenerated, or we need to offer pinned versions of netCDF (which I really very much don't want to do).

@Zeitsperre
Copy link
Member Author

@huard

It's looking good! The last thing that needs to be revised is the one failing test on macOS. I think it has to do with the CRS not being read on file open. One approach would be to change the engine being used to read the file (try GeoPandas?) Will explore this tomorrow.

@Zeitsperre Zeitsperre requested a review from huard May 25, 2023 17:31
@huard huard merged commit a9201c9 into master May 25, 2023
@huard huard deleted the adopt_pep517-621 branch May 25, 2023 20:50
Zeitsperre added a commit to Ouranosinc/raven that referenced this pull request Aug 17, 2023
## Overview

This PR fixes #472 

Changes:

* Modified entrypoint to use raven-wps
* use `python3` calls for compatibility (macOS and Linux)
* Updates the methods for fetching remote JSON files to be read by
GeoPandas (with pyogrio)

## Related Issue / Discussion

CSHS-CWRA/RavenPy#278 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Raven 3.7
2 participants