Warn if serializing non-virtual data (#657)

* test warning raised if writing non-virtual dataset to icechunk

* warn for Dataset

* generalize decorator to work for DataTree too

* test warned on datatree to icechunk

* lint

* test to_kerchunk

* release note

* avoid writing to same store twice in test

* remove decorator type hints

Author: TomNicholas

docs/releases.md

@@ -21,6 +21,8 @@
   `xarray.DataTree.virtualize.to_icechunk()` for writing an `xarray.DataTree`
   to an Icechunk store ([#244](https://github.com/zarr-developers/VirtualiZarr/issues/244)).  By
   [Chuck Daniels](https://github.com/chuckwondo).
+- Now throws a warning if you attempt to write an entirely non-virtual dataset to a virtual references format ([#657](https://github.com/zarr-developers/VirtualiZarr/pull/657)).
+  By [Tom Nicholas](https://github.com/TomNicholas).
 
 ### Breaking changes
 

virtualizarr/accessor.py

@@ -1,6 +1,16 @@
+import warnings
+from collections import deque
 from datetime import datetime
+from functools import wraps
 from pathlib import Path
-from typing import TYPE_CHECKING, Callable, Literal, overload
+from typing import (
+    TYPE_CHECKING,
+    Callable,
+    Generator,
+    Iterable,
+    Literal,
+    overload,
+)
 
 import xarray as xr
 
@@ -12,6 +22,44 @@
     from icechunk import IcechunkStore  # type: ignore[import-not-found]
 
 
+def warn_if_not_virtual(cls_name: Literal["Dataset", "DataTree"]):
+    """Decorator for methods which only make sense for fully virtual xarray objects."""
+
+    def decorator(func):
+        @wraps(func)
+        def wrapper(self, *args, **kwargs):
+            all_vars: Iterable[xr.Variable]
+            match cls_name:
+                case "Dataset":
+                    all_vars = self.ds.variables.values()
+                case "DataTree":
+                    all_vars = all_datatree_variables(self.dt)
+
+            if not any(isinstance(var.data, ManifestArray) for var in all_vars):
+                warnings.warn(
+                    f"Attempting to write an entirely non-virtual {cls_name} to a virtual references format - i.e. your `xarray.{cls_name}` contains zero `ManifestArray` objects. "
+                    "This is almost certainly not intended, as the entire data contents will be duplicated rather than referenced. "
+                    f"This may have happened because you used `xarray.open_{cls_name}` instead of `virtualizarr.open_virtual_{cls_name}`, or you set all variables to be `loadable_variables`."
+                    "Please read the usage docs.",
+                    UserWarning,
+                )
+
+            return func(self, *args, **kwargs)
+
+        return wrapper
+
+    return decorator
+
+
+def all_datatree_variables(root: xr.DataTree) -> Generator[xr.Variable, None, None]:
+    """Flat iterable over all variables in a DataTree"""
+    queue = deque([root])
+    while queue:
+        node = queue.popleft()
+        yield from node.variables.values()
+        queue.extend(node.children.values())
+
+
 @xr.register_dataset_accessor("virtualize")
 class VirtualiZarrDatasetAccessor:
     """
@@ -23,6 +71,7 @@ class VirtualiZarrDatasetAccessor:
     def __init__(self, ds: xr.Dataset):
         self.ds: xr.Dataset = ds
 
+    @warn_if_not_virtual("Dataset")
     def to_icechunk(
         self,
         store: "IcechunkStore",
@@ -93,6 +142,7 @@ def to_kerchunk(
         categorical_threshold: int = 10,
     ) -> None: ...
 
+    @warn_if_not_virtual("Dataset")
     def to_kerchunk(
         self,
         filepath: str | Path | None = None,
@@ -235,6 +285,7 @@ class VirtualiZarrDataTreeAccessor:
     def __init__(self, dt: xr.DataTree):
         self.dt = dt
 
+    @warn_if_not_virtual("DataTree")
     def to_icechunk(
         self,
         store: "IcechunkStore",

virtualizarr/tests/test_writers/test_icechunk.py

@@ -473,6 +473,23 @@ def test_roundtrip_coords(
     assert set(roundtrip.coords) == set(vds.coords)
 
 
+class TestWarnIfNotVirtual:
+    def test_warn_if_no_virtual_vars_dataset(self, icechunk_filestore: "IcechunkStore"):
+        non_virtual_ds = xr.Dataset({"foo": ("x", [10, 20, 30]), "x": ("x", [1, 2, 3])})
+        with pytest.warns(UserWarning, match="non-virtual"):
+            non_virtual_ds.virtualize.to_icechunk(icechunk_filestore)
+
+    def test_warn_if_no_virtual_vars_datatree(
+        self, icechunk_filestore: "IcechunkStore"
+    ):
+        non_virtual_ds = xr.Dataset({"foo": ("x", [10, 20, 30]), "x": ("x", [1, 2, 3])})
+        non_virtual_dt = xr.DataTree.from_dict(
+            {"/": non_virtual_ds, "/group": non_virtual_ds}
+        )
+        with pytest.warns(UserWarning, match="non-virtual"):
+            non_virtual_dt.virtualize.to_icechunk(icechunk_filestore)
+
+
 class TestAppend:
     """
     Tests for appending to existing icechunk store.

virtualizarr/tests/test_writers/test_kerchunk.py

@@ -1,5 +1,7 @@
 import numpy as np
 import pandas as pd
+import pytest
+import xarray as xr
 from xarray import Dataset
 from zarr.core.metadata.v2 import ArrayV2Metadata
 
@@ -179,3 +181,9 @@ def testconvert_v3_to_v2_metadata(array_v3_metadata):
     assert filters_config["dtype"] == "<i8"
     assert filters_config["astype"] == "<i8"
     assert v2_metadata.attributes == {}
+
+
+def test_warn_if_no_virtual_vars():
+    non_virtual_ds = xr.Dataset({"foo": ("x", [10, 20, 30]), "x": ("x", [1, 2, 3])})
+    with pytest.warns(UserWarning, match="non-virtual"):
+        non_virtual_ds.virtualize.to_kerchunk()