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.

Sign up for GitHub

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

Jump to bottom

Python 3.9: remove pynio as dependency and replace with rasterio and pin Matplotlib>3.3.1 and pin cartopy>=0.18 #1997

Merged
merged 26 commits into from
Jan 25, 2021
Merged

Python 3.9: remove pynio as dependency and replace with rasterio and pin Matplotlib>3.3.1 and pin cartopy>=0.18 #1997

merged 26 commits into from
Jan 25, 2021

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Jan 19, 2021 •
edited
Loading

Pynio removal
pynio is no longer maintained and is an obsolete package preventing us to support Python 3.9 as described in #1996 and a general nuisance as described in #1935
As far as the functionality in ESMValTool goes, it is used as a file loader engine for xarray in the cmorizer esmvaltool/cmorizers/obs/cmorize_obs_eppley_vgpm_modis.py script, but according to the xarray function documentation the engine arg is optional, and if not specified, the defaults netcdf4 is used and preferred. Any reason why the pynio engine is explicitly used? I don't have the rawOBS data so I can't test but if there is no particular reason for pynio use, please approve this PR; if there is a proper reason for pynio please mention it here and provide a usable and viable alternative. see below for rasterio

pynio -> rasterio

@tomaslovato examined the situation and recommended rasterio for an engine; rasterio is supported and on both conda-forge and PyPi (see pypi package). I have added it to the environment to be installed from PyPi and the environment builds well (without ESMValCore from PyPi, but installed in development mode, to test the Python 3.9 compatibility). rasterio wheel builds fine in a Python 3.9 environment and ESMValTool installs fine. @tomaslovato promised to include code for its use in the cmorizer script 👍

pin Matplotlib>3.3.1
I unpinned Matplotlib since we are not wanting Matplotlib 3.3.1 (the bug reported in Nikolay's comment) but 3.3.1 is already very old and 3.3.3 will be installed automatically. If we really are paranoid, we can force a condition !=3.3.1 but I don't think it's necessary. I have now pinned the lower bound to >3.3.1 to help the environment solve faster.

pin cartopy>=0.18
since Matplotlib 3 don't work with lower versions of cartopy

Replace conda env update with conda env create
In the Circle CI script I replaced calls to conda env update with full blown env creation conda env create, conda update is a bit hit and miss.

Changes from #1893
Fully ported here

Functionality for Python 3.9 use
Solves the env with esmvalcore out for full development installation (since the esmvalcore conda package is Python 3.8), installs OK and tests OK (Python 3.9.1 for conda=4.9.2)

Closes: #1935 #1996 #1998 #1893

@valeriupredoi valeriupredoi mentioned this pull request Jan 19, 2021
Closed
@valeriupredoi valeriupredoi changed the title Remove pynio as dependency and let cmorizer script use xarray with default engine Python 3.9: remove pynio as dependency and let cmorizer script use xarray with default engine and unpin Matplotlib Jan 20, 2021
@tomaslovato
Copy link
Contributor

Pynio removal
pynio is no longer maintained and is an obsolete package preventing us to support Python 3.9 as described in #1996 and a general nuisance as described in #1935
As far as the functionality in ESMValTool goes, it is used as a file loader engine for xarray in the cmorizer esmvaltool/cmorizers/obs/cmorize_obs_eppley_vgpm_modis.py script, but according to the xarray function documentation the engine arg is optional, and if not specified, the defaults netcdf4 is used and preferred. Any reason why the pynio engine is explicitly used? I don't have the rawOBS data so I can't test but if there is no particular reason for pynio use, please approve this PR; if there is a proper reason for pynio please mention it here and provide a usable and viable alternative.

@valeriupredoi As discussed in #1935, the xarray defaults netcdf4 is generally preferred, but it is not able to open and read HDF4 data (used e.g. for MODIS satellite data). The xarray documentation suggest to use a dedicated interface with the engine=pynio ( see xarray Reading and writing files).

After spending a bit of time in looking for viable alternatives, I was able to revise the 'indicted' cmorizer by using the xarray rasterio function to open the files and read the data

So the rasterio package can be a replacement for pynio ...
@valeriupredoi could you check if adding rasterio to the environment is ok?

If so, I'll be happy to update the cmorizer.

@valeriupredoi
Copy link
Contributor Author

@tomaslovato cheers for looking into this, mate, and proposing rasterio, I'll add it to the dependencies list here and test the environment creation and functionality (tomorrow) 👍

@tomaslovato
Copy link
Contributor

@valeriupredoi actually to use rasterio in the cmorizer it is necessary to change few lines of the code, not only the engine property.
If you want to add in this PR the requited changes to esmvaltool/cmorizers/obs/cmorize_obs_eppley_vgpm_modis.py
I can provide the new script using rasterio

@valeriupredoi
Copy link
Contributor Author

@tomaslovato could you pls commit here if it's not too long? Cheers 🍺

valeriupredoi and others added 2 commits January 21, 2021 17:40
@valeriupredoi
pin lower bound matplotlib
db9f727
@tomaslovato
Implement use of rasterio into cmorize_obs_eppley_vgpm_modis.py
bf7157a 
Usage of rasterio is required by the need to drop pynio package.
This imply the use of the xarray specific data read function open_rasterio,
but this requires some modification to the cmorizer code that involves
adjusting the informations flow from the input dataArray read by rasterio.
@tomaslovato
Copy link
Contributor

@valeriupredoi I added the required changes to the cmorizer code that produces a file that is exactly the same as in previous version (at least on our cluster!)

@valeriupredoi
Copy link
Contributor Author

You're a star, mate, cheers very much! Don't wait up on the tests, the environment creation takes a while. This is ready for merge, thanks @tomaslovato 🍺

bouweandela
bouweandela requested changes Jan 22, 2021
View reviewed changes
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Nice work!

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
environment.yml Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor Author

about @bouweandela 's suggestion to pin Cartopy - there is this PR #1893 that I will now include here, for a proper all-in-one test of env solvability etc, and then once we merge this one we can just close that one

@valeriupredoi
Copy link
Contributor Author

full port of #1893 here so I will close w/o merges that one when we merge this one 👍

@valeriupredoi valeriupredoi changed the title Python 3.9: remove pynio as dependency and let cmorizer script use xarray with default engine and unpin Matplotlib Python 3.9: remove pynio as dependency and let cmorizer script use xarray with default engine and unpin Matplotlib and pin cartopy Jan 25, 2021
@valeriupredoi valeriupredoi changed the title Python 3.9: remove pynio as dependency and let cmorizer script use xarray with default engine and unpin Matplotlib and pin cartopy Python 3.9: remove pynio as dependency and replace with rasterio and unpin Matplotlib and pin cartopy Jan 25, 2021
@valeriupredoi valeriupredoi changed the title Python 3.9: remove pynio as dependency and replace with rasterio and unpin Matplotlib and pin cartopy Python 3.9: remove pynio as dependency and replace with rasterio and pin Matplotlib>3.3.1 and pin cartopy>=0.18 Jan 25, 2021
@bouweandela bouweandela merged commit cb51bb8 into master Jan 25, 2021
@bouweandela bouweandela deleted the remove_pynio branch January 25, 2021 16:06
This was referenced Jan 25, 2021
Closed
Closed
@bouweandela bouweandela mentioned this pull request Oct 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bouweandela bouweandela bouweandela approved these changes

@jvegreg jvegreg Awaiting requested review from jvegreg

@tomaslovato tomaslovato Awaiting requested review from tomaslovato

Assignees
No one assigned
Labels
deprecated feature installation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove pynio as dependency
3 participants
@valeriupredoi @tomaslovato @bouweandela