Freeze xarray version given update causes crash #190
Conversation
New (24-9-0) xarray update causes error when calling bathymetry. A "strict" flag is added to one of the dask function calls which causes a crash. Need to freeze the version of xarray to fix for now, until we patch ahead of updating to new xarray version
pyproject.toml
Outdated
| @@ -14,7 +14,7 @@ dependencies = [ | |||
| "netCDF4", | |||
| "numpy >= 1.17.0, < 2.0.0", | |||
| "scipy >= 1.2.0", | |||
| "xarray", | |||
| "xarray == 2024.7.0", | |||
There was a problem hiding this comment.
I think there are older versions of xarray that also don’t work from memory. Safer to force it to one we know does work?
There was a problem hiding this comment.
What's the issue really? Is it documented? Or the error that one gets?
There was a problem hiding this comment.
Easy to reproduce, just run the tests like this one which used to pass before update
There was a problem hiding this comment.
The test you point above is from a different PR. So I'm confused @ashjbarnes. How do we know that the error in there is from the xarray version?
|
Can we rename the PR to better describe what happened? Eg something like “Add upper bound compatibility for xarray”? |
ashjbarnes
changed the title
navidcy
changed the title
There was a problem hiding this comment.
I'm happy to approve an upper bound compat requirement (e.g. <=X.Y.Z). It would be much better if the PR or an issue documented the precise error with xarray versions beyond 2024.7.0 so we know whether we fixed it in the future. At the moment the only info I see is "error when calling the bathymetry" but if I wanted to fix it I'm not sure what I should fix.
I think imposing strict lower bound compatibility just on the basis "from memory" is a bit too much. If at some point we find something that requires a lower bound compatibility we can address it then (either fixing it or imposing the lower bound then)?
What do you think @ashjbarnes?
|
Sure, I don't mean to argue I just think this PR in some form needs go through ASAP so that the package works again for new installs, and the main branch passes tests again. |
As I said:
Can we do that? |
|
I'm also happy for somebody else to review and ignore my comments! No problem! |
|
This PR closes issue #191 |
|
@ashjbarnes I reproduced the error and documented it in #191. |
New (24-9-0) xarray update causes error when calling bathymetry. A "strict" flag is added to one of the dask function calls which causes a crash. Need to freeze the version of xarray to fix for now, until we patch ahead of updating to new xarray version.
Bug found by @manishvenu