Import datatree in xarray? #7418

TomNicholas · 2023-01-04T20:48:09Z

I want datatree to live in xarray main, as right now it's in a separate package but imports many xarray internals.

This presents a few questions:

At what stage is datatree "ready" to moved in here? At what stage should it become encouraged public API?
What's a good way to slowly roll the feature out?
How do I decrease the bus factor on datatree's code? Can I get some code reviews during the merging process? 🙏
Should I make a new CI environment just for testing datatree stuff?

Today @jhamman and @keewis suggested for now I make it so that you can from xarray import DataTree, using the current xarray-datatree package as an optional dependency. That way I can create a smoother on-ramp, get some more users testing it, but without committing all the code into this repo yet.

@pydata/xarray what do you think? Any other thoughts about best practices when moving a good few thousand lines of code into xarray?

First step towards moving solution of Feature Request: Hierarchical storage and processing in xarray #4118 into this repository
Tests added
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

TomNicholas · 2023-01-04T20:50:20Z

xarray/__init__.py

+try:
+    from datatree import DataTree, open_datatree, register_datatree_accessor
+except ImportError:
+    ...


There is almost certainly a better way to make from xarray import DataTree work than this

I think this is fine as a temporary solution.

Illviljan · 2023-01-04T21:18:05Z

Add a py.typed file in datatree to fix the mypy error: https://github.com/pydata/xarray/blob/main/xarray/py.typed

TomNicholas · 2023-01-05T15:24:58Z

There is also at least one bug in datatree that cannot be fixed without a (small) change to xarray, and having datatree as an optional import means I could fix it here.

rabernat · 2023-01-05T21:31:33Z

At what stage is atatree "ready" to moved in here? At what stage should it become encouraged public API?

My opinion is that Datatree should move into Xarray now, ideally in a way that does not disrupt any existing user code, and that Datatree should become a first-class Xarray object (together with DataArray, and Dataset). Since it's a new feature, we don't necessarily have to be super conservative here. I think it is more than good enough / stable enough in its current state.

What's a good way to slowly roll the feature out?

Since Datatree sits above DataArray and Dataset, it should not interfere with any of our existing API. As long as test coverage is good, documentation is solid, and the code style matches the rest of Xarray, I think we can just bring it in.

How do I decrease the bus factor on datatree's code? Can I get some code reviews during the merging process? 🙏

I think that it is inevitable that you Tom will be the main owner of the Datatree code at the beginning (as @shoyer was of all of Xarray when he first released it). Over time, if people use it, some fraction of users will become maintainers, starting with the existing dev team.

Should I make a new CI environment just for testing datatree stuff?

Why? Are its dependencies different from Xarray?

TomNicholas · 2023-01-05T21:36:25Z

Why? Are its dependencies different from Xarray?

No, datatree has no additional dependencies. I was just asking because if we went for the "import from second repository" plan we may want to test that the import works as part of our CI. Not a major issue though.

rabernat · 2023-01-05T21:50:53Z

I personally favor just copying the code into Xarray and archiving the old repo.

jhamman · 2023-01-05T22:45:17Z

I personally favor just copying the code into Xarray and archiving the old repo.

I also lean in this direction. At this point, I see little downside to making this change at this point. My suggestion to import xarray-datatree into xarray was meant low-lift compromise.

benbovy · 2023-01-05T22:46:05Z

I don't have strong opinions for or against including datatree in Xarray. It indeed makes sense if it is using many Xarray internals and if there are many existing or potential applications for it. Additional load (CI) is fine if datatree doesn't bring any extra dependency and won't do so in the near future (which seems to be the case).

Datatree should become a first-class Xarray object

Since Datatree sits above DataArray and Dataset, it should not interfere with any of our existing API.

Would it mean that if someone wants to later add any feature "x" or "y" into Xarray, they just need implementing the feature for Dataset (and possibly DataArray) and it will be guaranteed to work with Datatree? (I guess so but I'm not familiar enough with Datatree to know it for sure).

Otherwise, if there is any extra implementation effort required to make feature "x" or "y" work with Datatree, then I'm concerned about the additional burden or obstacle for future contributors and maintainers. Or we could say that this is OK to leave datatree support and wait for someone to take care of it later, but I don't think it is ideal to have such non-synchronized state within Xarray itself.

benbovy · 2023-01-05T23:08:15Z

Again, there is likely more good reasons merging the Datatree code with Xarray than not doing it, but IMHO such decision should be made very carefully. You certainly do know better than me what positive vs. negative impacts it would have here! I'm just speaking generally from my experience of having struggled while doing some heavy refactoring in Xarray recently :)

TomNicholas · 2023-01-06T03:35:36Z

Would it mean that if someone wants to later add any feature "x" or "y" into Xarray, they just need implementing the feature for Dataset (and possibly DataArray) and it will be guaranteed to work with Datatree?

Basically yes, it would immediately work with Datatree. Datatree currently implements most dataset methods by literally copying them and their docstrings, and they work by mapping the method over every node in the tree. We could integrate Datatree in such a way that the additional developer effort to get a method on dataset working on Datatree would be negligible (think adding a single element with the method name to an internal list, or copy-pasting a docstring).

I don't think it is ideal to have such non-synchronized state within Xarray itself.

This is an argument for waiting before integrating.

I'm just speaking generally from my experience of having struggled while doing some heavy refactoring in Xarray recently :)

I appreciate the input @benbovy! I think the main difference between this effort and your (heroic) indexes effort is that Datatree doesn't touch any existing API.

I guess my main concern is that integrating prematurely into Xarray might give a false sense of stability - I don't want to later realize I should redesign Datatree, and have people be annoyed because they thought it was as stable as the rest of xarray.

TomNicholas · 2023-01-10T21:33:27Z

Integrating upstream into xarray might also help with people trying to open their nested data formats as a datatree objects, because then we can immediately begin integrating with xarray's backend engines.

See for example this datatree issue asking about opening grib files as a datatree. It would be nice to be able to do

open_datatree("data.grib", engine="cfgrib")

shoyer · 2023-01-11T00:27:47Z

I agree, datatree is an important data structure for Xarray. My preferred way to do this would be follow @rabernat's suggestion and to fork the code the existing repo into the Xarray main codebase.

My main concern is that we should carefully evaluate the datatree API to make sure we won't want to change it soon. Once we bring it into Xarray, there will be a higher expectation that the interface will remain stable.

rabernat · 2023-01-11T00:34:03Z

we should carefully evaluate the datatree API to make sure we won't want to change it soon

I agree with this. We could use the PR process for this review.

for more information, see https://pre-commit.ci

… into import_datatree

for more information, see https://pre-commit.ci

… into import_datatree

TomNicholas · 2023-02-01T04:40:30Z

I think this PR is ready now, it just fails mypy (@Illviljan I added a py.typed file to datatree but the xarray mypy CI is still not happy with it).

Once this is merged we can push on with implementing open_datatree (#7437 @jthielen) and I can take a crack at fixing xarray-contrib/datatree#146 in xarray in a follow-up PR.

Illviljan · 2023-02-01T06:34:08Z

Hmm, I don't understand. Adding py.typed should be all that's needed, did that in flox and it worked great there: xarray-contrib/flox#92

keewis

My suggestion is to follow Tom's plan to simply import datatree for 1-2 release cycles.

sounds good to me!

xarray/__init__.py

keewis · 2023-02-01T12:04:37Z

xarray/__init__.py

@@ -52,6 +52,12 @@
    # Disable minimum version checks on downstream libraries.
    __version__ = "999"

+try:
+    from datatree import DataTree  # noqa


Suggested change

from datatree import DataTree # noqa

from datatree import DataTree, register_datatree_accessors # noqa: F401

I think this is why the docs build is failing. Also, not sure if the error code still works with ruff?

Also, not sure if the error code still works with ruff?

I actually found that if I don't add that error code then ruff replaces

try: from datatree import DataTree

with

try: pass

which obviously caused an ImportError. I thought that was surprising behavior for a linter too...

I've changed this to

from datatree import DataTree, register_datatree_accessor, open_datatree # noqa

now though

keewis · 2023-02-01T12:09:39Z

xarray/tests/test_datatree.py

+import xarray.testing as xrt
+from xarray import Dataset
+from xarray.tests import requires_datatree
+


since the whole module depends on datatree, I'd call pytest.importorskip("datatree") somewhere at the top of the module:

Suggested change

pytest.importorskip("datatree")

then we don't need to decorate every test with requires_datatree

If we want to reuse requires_datatree, we can use:

Suggested change

pytestmark = [requires_datatree]

keewis · 2023-02-01T12:14:52Z

xarray/core/dataset.py

+        .. warning:: The DataTree structure is considered experimental,
+            and the API is less solidified than for other xarray features.


not sure if I just don't know enough about rst, but I wonder if it would be better to move the whole text into the block?

Suggested change

.. warning:: The DataTree structure is considered experimental,

and the API is less solidified than for other xarray features.

.. warning::

The DataTree structure is considered experimental, and the API

is less solidified than for other xarray features.

keewis · 2023-02-01T12:15:37Z

xarray/core/dataarray.py

+        WARNING: The DataTree structure is considered experimental,
+        and the API is less solidified than for other xarray features.
+
+        The returned tree will only consist of a single node.
+        That node will contain a copy of the dataarray's data,
+        meaning including its coordinates, dimensions and attributes.
+
+        Requires the xarray-datatree package to be installed.
+        Find it at https://github.com/xarray-contrib/datatree.


should this also be moved into a warning block?

keewis · 2023-02-01T12:17:55Z

doc/user-guide/io.rst

+.. _netcdf.group.warning:
+
+.. warning::
+    ``DataTree`` objects do not follow the exact same data model as netCDF files, which means that perfect round-tripping


Is that intentionally preformatted, or would it make sense to convert it to a link? (that's really minor, though)

keewis · 2023-02-01T12:21:54Z

ci/requirements/environment.yml

+  - pip:
+    - git+https://github.com/xarray-contrib/datatree


is there a reason why we're installing from github here?

Because I want to see if this commit to datatree fixes the mypy issue without releasing a whole new version of datatree just to check.

keewis · 2023-02-01T12:22:18Z

ci/install-upstream-wheels.sh

@@ -20,6 +20,7 @@ conda uninstall -y --force \
    bottleneck \
    sparse \
    flox \
+    datatree \


it's probably fine as-is (and I'm always confused about the name), but should this be xarray-datatree, given that that's the package we are installing below?

Co-authored-by: Justus Magin <keewis@users.noreply.github.com>

TomNicholas · 2023-11-09T00:12:02Z

So I let this get stale, but today I met with @owenlittlejohns and @asteiker who are both very interested in helping get datatree merged into xarray upstream, which is great!

I would like to:

forget about this PR, and just begin moving datatree upstream,
use the merging to get good feedback on the design of datatree instead of just copy-pasting,
use the merging as an opportunity to reduce the bus factor on datatree's code.

@owenlittlejohns especially seemed interested in actually understanding the guts of datatree.

It would be helpful if at least one other @pydata/xarray dev was interested and had time to review such PRs, so that I'm not just approving merging my own code. @jhamman ? @shoyer ? @keewis ? You guys have been particularly helpful with datatree so far.

owenlittlejohns · 2023-12-05T20:00:04Z

@flamingbear, @eni-awowale and @lsterzinger - I just wanted to tag you here as a starting point for discussing this effort a bit more, as you all were interested in helping with this work.

TomNicholas · 2023-12-06T18:05:15Z

Thanks for coming to the meeting today @owenlittlejohns , @eni-awowale and @flamingbear!

We decided that opening hierarchical files into xarray is the first priority, so ideally integration of datatree into xarray should start with the backends. There is a prototype of this in #7437, but that's gone stale (cc @jthielen ?) so I think we should just use it as reference and forge ahead!

That would give us a temporary situation in which xarray.open_datatree(file) returned a datatree.DataTree object, as a stepping stone towards returning an xarray.DataTree object.

TomNicholas · 2023-12-22T17:38:04Z

Closing this in favour of tracking a proper integration in #8572.

TomNicholas added 3 commits January 4, 2023 15:20

list datatree in public API

7c6fa70

attempt to import datatree API on xarray import

5ef43be

incorporate datatree links into io docs on groups

d184764

TomNicholas added topic-DataTree Related to the implementation of a DataTree class dependencies Pull requests that update a dependency file labels Jan 4, 2023

github-actions bot removed topic-DataTree Related to the implementation of a DataTree class dependencies Pull requests that update a dependency file labels Jan 4, 2023

TomNicholas commented Jan 4, 2023

View reviewed changes

TomNicholas added dependencies Pull requests that update a dependency file topic-DataTree Related to the implementation of a DataTree class labels Jan 4, 2023

Merge branch 'main' into import_datatree

d986df3

github-actions bot removed topic-DataTree Related to the implementation of a DataTree class dependencies Pull requests that update a dependency file labels Jan 4, 2023

rabernat mentioned this pull request Jan 6, 2023

Add printable representation for MDIOReader and MDIOWriter TGSAI/mdio-python#33

Open

TomNicholas mentioned this pull request Jan 10, 2023

Idea/use case: cfgrib xarray-contrib/datatree#195

Closed

jthielen mentioned this pull request Jan 10, 2023

Support DataTree for organizing Datasets by type of level ecmwf/cfgrib#327

Open

abkfenris mentioned this pull request Jan 12, 2023

Using datatrees to represent datasets pangeo-data/climpred#809

Open

add Dataset.to_datatree() method

d2e8ec3

pre-commit-ci bot and others added 3 commits February 1, 2023 00:31

[pre-commit.ci] auto fixes from pre-commit.com hooks

95d76e6

for more information, see https://pre-commit.ci

test .to_datatree method

caafe90

Merge branch 'import_datatree' of https://github.com/TomNicholas/xarray…

462e0b3

… into import_datatree

github-actions bot added the topic-typing label Feb 1, 2023

pre-commit-ci bot and others added 6 commits February 1, 2023 01:01

[pre-commit.ci] auto fixes from pre-commit.com hooks

91c6ee1

for more information, see https://pre-commit.ci

fix datatree import

bc6a538

[pre-commit.ci] auto fixes from pre-commit.com hooks

3baf79e

for more information, see https://pre-commit.ci

protect my import from the exacting ruff linter

667d5cd

Merge branch 'import_datatree' of https://github.com/TomNicholas/xarray…

dfe763b

… into import_datatree

try installing datatree from main

d231055

TomNicholas mentioned this pull request Feb 1, 2023

Enable datatree * dataset commutativity #7497

Closed

4 tasks

keewis reviewed Feb 1, 2023

View reviewed changes

TomNicholas and others added 4 commits February 1, 2023 11:12

Update xarray/__init__.py

ae07dfd

Co-authored-by: Justus Magin <keewis@users.noreply.github.com>

also import accessor and open_datatree in top-level init

6343104

importorskip whole test file

395a3ae

correct package name in wheels

7cf1d55

This was referenced Feb 2, 2023

[BUG] Broken CI due to setuptools error xarray-contrib/datatree#216

Closed

Add a .to_node method upstream xarray-contrib/datatree#191

Closed

weiji14 mentioned this pull request Feb 19, 2023

Slicing multiple data layers or band channels with different spatial resolutions xarray-contrib/xbatcher#93

Open

TomNicholas added the topic-DataTree Related to the implementation of a DataTree class label Dec 6, 2023

TomNicholas mentioned this pull request Dec 22, 2023

Track merging datatree into xarray #8572

Closed

27 tasks

TomNicholas closed this Dec 22, 2023

droumis mentioned this pull request Jan 5, 2024

[GOAL] Demo viewing of large multi-chan timeseries data with multi-time-resolution generator and dynamic accessor holoviz-topics/neuro#87

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import datatree in xarray? #7418

Import datatree in xarray? #7418

TomNicholas commented Jan 4, 2023 •

edited

Loading

TomNicholas Jan 4, 2023

jhamman Jan 31, 2023

Illviljan commented Jan 4, 2023

TomNicholas commented Jan 5, 2023

rabernat commented Jan 5, 2023

TomNicholas commented Jan 5, 2023

rabernat commented Jan 5, 2023

jhamman commented Jan 5, 2023

benbovy commented Jan 5, 2023

benbovy commented Jan 5, 2023

TomNicholas commented Jan 6, 2023

TomNicholas commented Jan 10, 2023

shoyer commented Jan 11, 2023

rabernat commented Jan 11, 2023

TomNicholas commented Feb 1, 2023

Illviljan commented Feb 1, 2023

keewis left a comment •

edited

Loading

keewis Feb 1, 2023

TomNicholas Feb 1, 2023

TomNicholas Feb 1, 2023

keewis Feb 1, 2023

keewis Feb 1, 2023

keewis Feb 1, 2023

keewis Feb 1, 2023

keewis Feb 1, 2023

TomNicholas Feb 1, 2023 •

edited

Loading

keewis Feb 1, 2023

TomNicholas Feb 1, 2023

TomNicholas commented Nov 9, 2023

owenlittlejohns commented Dec 5, 2023

TomNicholas commented Dec 6, 2023

TomNicholas commented Dec 22, 2023

	from datatree import DataTree # noqa
	from datatree import DataTree, register_datatree_accessors # noqa: F401

		.. warning:: The DataTree structure is considered experimental,
		and the API is less solidified than for other xarray features.

Import datatree in xarray? #7418

Import datatree in xarray? #7418

Conversation

TomNicholas commented Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Illviljan commented Jan 4, 2023

TomNicholas commented Jan 5, 2023

rabernat commented Jan 5, 2023

TomNicholas commented Jan 5, 2023

rabernat commented Jan 5, 2023

jhamman commented Jan 5, 2023

benbovy commented Jan 5, 2023

benbovy commented Jan 5, 2023

TomNicholas commented Jan 6, 2023

TomNicholas commented Jan 10, 2023

shoyer commented Jan 11, 2023

rabernat commented Jan 11, 2023

TomNicholas commented Feb 1, 2023

Illviljan commented Feb 1, 2023

keewis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas commented Nov 9, 2023

owenlittlejohns commented Dec 5, 2023

TomNicholas commented Dec 6, 2023

TomNicholas commented Dec 22, 2023

TomNicholas commented Jan 4, 2023 •

edited

Loading

keewis left a comment •

edited

Loading

TomNicholas Feb 1, 2023 •

edited

Loading