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

FIX: Ensure backwards compatibility with datatree/new versions of xarray #1681

Merged
merged 7 commits into from
Nov 8, 2024

Conversation

juhi24
Copy link
Contributor

@juhi24 juhi24 commented Nov 5, 2024

xarray-datatree has been merged to xarray>=2024.10.0. Since xarray version is not pinned, this will cause ImportErrors unless addressed. This is PR is an attempt towards a phased transition to using xarray.datatree.

Copy link
Contributor

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

This looks like a step in the right direction, however if neither xarray-datatree nor datatree is a hard dependency to pyart, then there should probably be an abstract way to specify which flavor users want (e.g. pip install arm-pyart[xarray-datatree], pip install arm-pyart[datatree]). This would preserve downstream from having to specify the exact requirements themselves, though it doesn't address the fact that the default install is broken.
I would recommend having xarray-datatree as a hard dependency and be done with it, but if that is not possible I would still recommend crafting a custom error message to explain the situation when both import fail

@juhi24
Copy link
Contributor Author

juhi24 commented Nov 6, 2024

You probably meant packages xarray and xarray-datatree, providing modules xarray.datatree and datatree, respectively. Package called datatree is unrelated. The package xarray-datatree is deprecated in favour of xarray, which now includes the datatree functionality. I would recommend against providing a flavour based on a package that is no longer maintained.

Currently, pyart seems to have a hard dependency on (the latest) xradar which, in turn, depends on xarray >= 2024.10.0. Therefore, pyart should either use xarray.datatree or pin to earlier versions of xradar and xarray. Otherwise, the default installation will remain broken.

@kmuehlbauer
Copy link
Contributor

From my perspective it would be reasonable to:

  1. pin xradar<=0.7.0 and xarray<=2024.9.0 and do a release
  2. fix anything which has to be fixed
  3. pin xradar>=0.8.0 and xarray>=2024.10.0 and do a another release

That's one way to not break users existing code. Users might then decide which version to use.

The old xarray-datatree and the new datatree from xarray differ in quite some places and would need users to adapt their code (depending on what they've imported from there). I'm not that familiar with pyart's usage of xradar, but good chance that correct pinning will already do the trick.

@juhi24
Copy link
Contributor Author

juhi24 commented Nov 7, 2024

Here's also the offical migration guide for reference https://github.com/pydata/xarray/blob/main/DATATREE_MIGRATION_GUIDE.md

@mgrover1
Copy link
Collaborator

mgrover1 commented Nov 7, 2024

Thank for the PR here @juhi24 !! I have opened another PR (#1682) which should help toward fixing this issue. I think we should:

  • Merge that one
  • Cut a new release
  • Fix the failing CI on this PR, and merge this
  • Release again with the updated xarray/xradar releases

@mgrover1 mgrover1 changed the title try to use datatree from xarray FIX: Ensure backwards compatibility with datatree/new versions of xarray Nov 7, 2024
@mgrover1 mgrover1 merged commit 699f961 into ARM-DOE:main Nov 8, 2024
15 checks passed
@juhi24 juhi24 deleted the datatree branch November 8, 2024 20:51
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.

4 participants