From b9336ba379b6193ee3c9959f447eb587f3b96ff9 Mon Sep 17 00:00:00 2001 From: wpbonelli Date: Mon, 24 Nov 2025 04:28:32 -0500 Subject: [PATCH 1/3] make Component.load a classmethod --- flopy4/mf6/__init__.py | 9 +- flopy4/mf6/component.py | 13 +- flopy4/mf6/context.py | 18 +- flopy4/uio.py | 12 +- test/IO_PLUMBING_TESTS_README.md | 131 +++++++++ test/test_io_plumbing.py | 490 +++++++++++++++++++++++++++++++ 6 files changed, 653 insertions(+), 20 deletions(-) create mode 100644 test/IO_PLUMBING_TESTS_README.md create mode 100644 test/test_io_plumbing.py diff --git a/flopy4/mf6/__init__.py b/flopy4/mf6/__init__.py index 45ae39b5..5e835eb4 100644 --- a/flopy4/mf6/__init__.py +++ b/flopy4/mf6/__init__.py @@ -25,17 +25,20 @@ class WriteError(Exception): pass -def _load_mf6(path: Path) -> Component: +def _load_mf6(cls, path: Path) -> Component: + """Load MF6 format file into a component instance.""" with open(path, "r") as fp: return structure(load_mf6(fp), path) -def _load_json(path: Path) -> Component: +def _load_json(cls, path: Path) -> Component: + """Load JSON format file into a component instance.""" with open(path, "r") as fp: return structure(load_json(fp), path) -def _load_toml(path: Path) -> Component: +def _load_toml(cls, path: Path) -> Component: + """Load TOML format file into a component instance.""" with open(path, "rb") as fp: return structure(load_toml(fp), path) diff --git a/flopy4/mf6/component.py b/flopy4/mf6/component.py index 56bd523a..8cf05985 100644 --- a/flopy4/mf6/component.py +++ b/flopy4/mf6/component.py @@ -1,5 +1,6 @@ from abc import ABC from collections.abc import MutableMapping +from os import PathLike from pathlib import Path from typing import Any, ClassVar, Optional @@ -132,16 +133,12 @@ def get_dfn(cls) -> Dfn: blocks=blocks, ) - def load(self, format: str = MF6) -> None: + @classmethod + def load(cls, path: str | PathLike, format: str = MF6) -> None: """Load the component and any children.""" - # TODO: setting filename is a temp hack to get the parent's - # name as this component's filename stem, if it has one. an - # actual solution is to auto-set the filename when children - # are attached to parents. - self.filename = self.filename or self.default_filename() - self._load(format=format) + self = cls._load(path, format=format) # Get the instance for child in self.children.values(): # type: ignore - child.load(format=format) + child.__class__.load(child.path, format=format) def write(self, format: str = MF6, context: Optional[WriteContext] = None) -> None: """ diff --git a/flopy4/mf6/context.py b/flopy4/mf6/context.py index e86b3f46..8df3fa54 100644 --- a/flopy4/mf6/context.py +++ b/flopy4/mf6/context.py @@ -48,9 +48,21 @@ def path(self) -> Path: self.filename = self.filename or self.default_filename() return self.workspace / self.filename - def load(self, format=MF6): - with cd(self.workspace): - super().load(format=format) + @classmethod + def load(cls, path, format=MF6): + """ + Load the context component and children. + + Children are loaded relative to the parent's workspace directory, + so their paths are resolved within that workspace. + """ + # Load the instance first + instance = cls._load(path, format=format) + + # Load children within the workspace context + with cd(instance.workspace): + for child in instance.children.values(): # type: ignore + child.__class__.load(child.path, format=format) def write(self, format=MF6, context=None): with cd(self.workspace): diff --git a/flopy4/uio.py b/flopy4/uio.py index a9d38b38..f68d6f3b 100644 --- a/flopy4/uio.py +++ b/flopy4/uio.py @@ -55,9 +55,9 @@ def register_writer(self, cls, format, function): raise ValueError(f"Writer for format {format} already registered.") self._writers[cls, format] = function - def load(self, cls, instance, *args, format=None, **kwargs): + def load(self, cls, *args, format=None, **kwargs): _load = self.get_loader(cls, format) - return _load(instance, *args, **kwargs) + return _load(cls, *args, **kwargs) def write(self, cls, instance, *args, format=None, **kwargs): _write = self.get_writer(cls, format) @@ -76,8 +76,8 @@ class IO(property): See the `astropy` source for more details/motivation. """ - def __get__(self, instance, owner_cls): - return self.fget(instance, owner_cls) + def __get__(self, instance, cls): + return self.fget(instance, cls) class IODescriptor: @@ -85,7 +85,7 @@ class IODescriptor: def __init__(self, instance, cls, op: Op, registry: Registry | None = None): self._registry = registry or DEFAULT_REGISTRY - self._instance = instance + self._instance = instance # None for loaders self._cls = cls self._op: Op = op @@ -111,7 +111,7 @@ def __init__(self, instance, cls): super().__init__(instance, cls, "load", registry=DEFAULT_REGISTRY) def __call__(self, *args, **kwargs): - return self.registry.load(self._cls, self._instance, *args, **kwargs) + return self.registry.load(self._cls, *args, **kwargs) class Writer(IODescriptor): diff --git a/test/IO_PLUMBING_TESTS_README.md b/test/IO_PLUMBING_TESTS_README.md new file mode 100644 index 00000000..20f6b1ef --- /dev/null +++ b/test/IO_PLUMBING_TESTS_README.md @@ -0,0 +1,131 @@ +# IO Plumbing Tests for Classmethod Load + +## Overview + +This test suite (`test_io_plumbing.py`) validates the IO plumbing layer for the refactored `Component.load()` classmethod. The tests ensure that: + +1. IO descriptors work correctly in classmethod contexts +2. The registry can register and lookup loaders/writers +3. Loaders can be invoked from class methods (not just instances) +4. The plumbing layer properly supports inheritance +5. Child components are loaded recursively +6. Context components load children relative to their workspace + +## Test Coverage (16 Tests) + +### Core Descriptor Tests + +- **`test_io_descriptor_access_from_class()`**: Verifies that `_load` descriptor can be accessed from a class (without an instance) +- **`test_io_descriptor_access_from_instance()`**: Verifies that `_load` descriptor still works with instances +- **`test_io_descriptor_callable()`**: Confirms the descriptor returns a callable Loader object + +### Registry Tests + +- **`test_loader_registry_lookup()`**: Tests basic loader registration and lookup +- **`test_loader_registry_subclass_lookup()`**: Verifies that loaders registered for base classes work with subclasses +- **`test_multiple_format_registrations()`**: Tests that different loaders can be registered for different formats + +### Classmethod Integration Tests + +- **`test_classmethod_load_with_mock_loader()`**: Full integration test showing loader invocation from classmethod context +- **`test_classmethod_load_signature()`**: Validates the `load()` method signature +- **`test_classmethod_load_should_return_instance()`**: Demonstrates expected behavior (loader should return an instance) +- **`test_loader_can_be_called_directly_from_class()`**: Tests the exact pattern used in `Component.load()` +- **`test_component_load_classmethod_calls_loader()`**: Verifies that `Component.load()` correctly invokes registered loaders +- **`test_component_load_with_children()`**: Tests that children are loaded recursively + +### Context/Workspace Tests + +- **`test_context_load_with_workspace()`**: Verifies that `Context.load()` loads children relative to the parent's workspace directory + +### Writer Tests + +- **`test_writer_descriptor_requires_instance()`**: Verifies writers work correctly with instances +- **`test_registry_write_with_mock_writer()`**: Tests writer registration and invocation + +### Type Annotation Tests + +- **`test_load_return_type()`**: Validates the return type annotation of the `load()` method + +## Issues Fixed + +### ✅ Loader Signature Mismatch (flopy4/mf6/__init__.py) + +**Problem**: Loader functions had old signature without `cls` parameter: +```python +def _load_mf6(path: Path) -> Component: # ❌ Missing cls parameter +``` + +**Fixed**: Updated all loaders to accept `cls` as first parameter: +```python +def _load_mf6(cls, path: Path) -> Component: # ✅ Correct signature +def _load_json(cls, path: Path) -> Component: +def _load_toml(cls, path: Path) -> Component: +``` + +### ✅ Bug in Component.load() (flopy4/mf6/component.py:139-140) + +**Problem**: Used undefined `self` in classmethod context: +```python +@classmethod +def load(cls, path: str | PathLike, format: str = MF6) -> None: + cls._load(path, format=format) + for child in self.children.values(): # ❌ self doesn't exist +``` + +**Fixed**: Assign loader result to `self`: +```python +@classmethod +def load(cls, path: str | PathLike, format: str = MF6) -> None: + self = cls._load(path, format=format) # ✅ Get the instance + for child in self.children.values(): + child.__class__.load(child.path, format=format) +``` + +### ✅ Context.load() Workspace Support (flopy4/mf6/context.py) + +**Problem**: `Context.load()` was an instance method and didn't use workspace for children: +```python +def load(self, format=MF6): # ❌ Instance method + with cd(self.workspace): + super().load(format=format) +``` + +**Fixed**: Made it a classmethod that loads children within workspace: +```python +@classmethod +def load(cls, path, format=MF6): # ✅ Classmethod + """Load context and children relative to workspace.""" + instance = cls._load(path, format=format) + + # Load children within the workspace context + with cd(instance.workspace): + for child in instance.children.values(): + child.__class__.load(child.path, format=format) +``` + +Now child components are loaded with paths relative to the parent's workspace directory, not the current working directory. + +## Running the Tests + +```bash +pixi run -e dev pytest test/test_io_plumbing.py -v +``` + +All 16 tests should pass ✅ + +## Summary + +The IO plumbing is now fully functional for classmethod-based loading: + +1. ✅ Loader functions have correct signature with `cls` parameter +2. ✅ `Component.load()` correctly loads parent and children +3. ✅ `Context.load()` loads children relative to parent's workspace +4. ✅ Registry properly looks up loaders for classes and subclasses +5. ✅ All descriptors work in both class and instance contexts + +## Next Steps + +1. Wire up the actual loader implementations to the converter/transformer layers +2. Add integration tests with real MF6 files +3. Consider whether `load()` should return the loaded instance for user convenience diff --git a/test/test_io_plumbing.py b/test/test_io_plumbing.py new file mode 100644 index 00000000..48c3b03b --- /dev/null +++ b/test/test_io_plumbing.py @@ -0,0 +1,490 @@ +"""Test IO plumbing for classmethod load/write operations.""" + +from pathlib import Path + +from xattree import xattree + +from flopy4.mf6.component import Component +from flopy4.mf6.constants import MF6 +from flopy4.mf6.context import Context +from flopy4.uio import DEFAULT_REGISTRY, IO, Loader, Registry, Writer + + +@xattree +class MockComponent(Component): + """Minimal test component for IO testing.""" + + name: str = "test" + + +def test_io_descriptor_access_from_class(): + """Test that IO descriptors can be accessed from a class (not instance).""" + # Access the descriptor from the class + loader = MockComponent._load + + # Should return a Loader instance + assert isinstance(loader, Loader) + assert loader._cls is MockComponent + assert loader._instance is None # No instance in classmethod context + + +def test_io_descriptor_access_from_instance(): + """Test that IO descriptors can still be accessed from an instance.""" + component = MockComponent() + loader = component._load + + # Should return a Loader instance + assert isinstance(loader, Loader) + assert loader._cls is MockComponent + assert loader._instance is component + + +def test_classmethod_load_with_mock_loader(): + """Test that Component.load() classmethod can invoke a registered loader.""" + # Create a separate registry for testing to avoid polluting the global one + test_registry = Registry() + + # Track whether the loader was called + load_called = {"value": False, "cls": None, "path": None, "format": None} + + def mock_loader(cls, path, format=MF6): + """Mock loader function that tracks invocation.""" + load_called["value"] = True + load_called["cls"] = cls + load_called["path"] = path + load_called["format"] = format + # Return a new instance + return cls(name="loaded") + + # Register the mock loader + test_registry.register_loader(MockComponent, MF6, mock_loader) + + # Replace the descriptor's registry temporarily + original_load = MockComponent._load + MockComponent._load = IO(lambda instance, cls: Loader(instance, cls)) + + # Manually create a loader with the test registry + loader = Loader(None, MockComponent) + loader._registry = test_registry + + # Call the loader + test_path = Path("/test/path.txt") + result = loader(test_path, format=MF6) + + # Verify the mock loader was called correctly + assert load_called["value"] + assert load_called["cls"] is MockComponent + assert load_called["path"] == test_path + assert load_called["format"] == MF6 + assert result.name == "loaded" + + # Restore original + MockComponent._load = original_load + + +def test_loader_registry_lookup(): + """Test that the registry can look up the correct loader function.""" + test_registry = Registry() + + def loader_fn(cls, path, format=MF6): + return cls() + + # Register loader + test_registry.register_loader(MockComponent, MF6, loader_fn) + + # Lookup loader + found_loader = test_registry.get_loader(MockComponent, format=MF6) + assert found_loader is loader_fn + + +def test_loader_registry_subclass_lookup(): + """Test that registry correctly finds loaders for subclasses.""" + test_registry = Registry() + + @xattree + class SubComponent(MockComponent): + """Subclass of MockComponent.""" + + pass + + def base_loader(cls, path, format=MF6): + return cls() + + # Register loader for base class + test_registry.register_loader(MockComponent, MF6, base_loader) + + # Should find loader for subclass via issubclass check + found_loader = test_registry.get_loader(SubComponent, format=MF6) + assert found_loader is base_loader + + +def test_writer_descriptor_requires_instance(): + """Test that Writer descriptor works with instances.""" + component = MockComponent(name="test_write") + writer = component._write + + # Should return a Writer instance with the component instance + assert isinstance(writer, Writer) + assert writer._cls is MockComponent + assert writer._instance is component + + +def test_classmethod_load_signature(): + """Test that the load classmethod has the expected signature.""" + import inspect + + # Get the load method + load_method = MockComponent.load + + # Check it's a classmethod + assert isinstance(inspect.getattr_static(MockComponent, "load"), classmethod) + + # Check signature + sig = inspect.signature(load_method) + params = list(sig.parameters.keys()) + + # Should have: cls (implicit), path, format + assert "path" in params + assert "format" in params + + +def test_io_descriptor_callable(): + """Test that the Loader descriptor returns a callable object.""" + loader = MockComponent._load + + # Should be callable + assert callable(loader) + + +def test_registry_write_with_mock_writer(): + """Test that the Writer can invoke a registered writer function.""" + test_registry = Registry() + + # Track whether the writer was called + write_called = {"value": False, "instance": None, "format": None} + + def mock_writer(instance, format=MF6, context=None): + """Mock writer function that tracks invocation.""" + write_called["value"] = True + write_called["instance"] = instance + write_called["format"] = format + + # Register the mock writer + test_registry.register_writer(MockComponent, MF6, mock_writer) + + # Create instance and writer + component = MockComponent(name="test") + writer = Writer(component, MockComponent) + writer._registry = test_registry + + # Call the writer + writer(format=MF6) + + # Verify the mock writer was called correctly + assert write_called["value"] + assert write_called["instance"] is component + assert write_called["format"] == MF6 + + +def test_load_return_type(): + """Test that load method returns the correct type hint.""" + import inspect + + sig = inspect.signature(MockComponent.load) + return_annotation = sig.return_annotation + + # Check the return annotation - should be None (the value) + assert return_annotation is None + + +def test_multiple_format_registrations(): + """Test that different loaders can be registered for different formats.""" + test_registry = Registry() + + format1_called = {"value": False} + format2_called = {"value": False} + + def loader_fmt1(cls, path, format=None): + format1_called["value"] = True + return cls() + + def loader_fmt2(cls, path, format=None): + format2_called["value"] = True + return cls() + + # Register different loaders for different formats + test_registry.register_loader(MockComponent, "format1", loader_fmt1) + test_registry.register_loader(MockComponent, "format2", loader_fmt2) + + # Get format1 loader + loader1 = test_registry.get_loader(MockComponent, format="format1") + assert loader1 is loader_fmt1 + + # Get format2 loader + loader2 = test_registry.get_loader(MockComponent, format="format2") + assert loader2 is loader_fmt2 + + +def test_classmethod_load_should_return_instance(): + """ + Test that demonstrates the expected behavior for load() as a classmethod. + + The current implementation has a bug at component.py:140 where it uses + `self.children` in a classmethod context. This test shows the expected + pattern: load() should return an instance so that children can be accessed. + """ + test_registry = Registry() + + def mock_loader(cls, path, format=MF6): + """Mock loader that returns a new instance.""" + return cls(name="loaded_from_file") + + # Register the loader + test_registry.register_loader(MockComponent, MF6, mock_loader) + + # Create a loader with the test registry + loader = Loader(None, MockComponent) + loader._registry = test_registry + + # Call the loader - should return an instance + test_path = Path("/test/path.txt") + result = loader(test_path, format=MF6) + + # Verify we got an instance back + assert isinstance(result, MockComponent) + assert result.name == "loaded_from_file" + + # This is what load() should do: return the instance + # so that code like `component = Component.load(path)` works + # and children can be accessed via `component.children` + + +def test_loader_can_be_called_directly_from_class(): + """ + Test that the loader can be accessed and called directly from a class. + This is the pattern that Component.load() uses. + """ + test_registry = Registry() + + # Track calls + calls = [] + + def mock_loader(cls, path, format=MF6): + calls.append({"cls": cls, "path": path, "format": format}) + return cls(name="loaded") + + # Register loader + test_registry.register_loader(MockComponent, MF6, mock_loader) + + # Simulate what Component.load() does: access descriptor from class + loader = MockComponent._load + loader._registry = test_registry + + # Call it with a path + result = loader(Path("/test/file.txt"), format=MF6) + + # Verify the loader was called correctly + assert len(calls) == 1 + assert calls[0]["cls"] is MockComponent + assert calls[0]["path"] == Path("/test/file.txt") + assert calls[0]["format"] == MF6 + assert isinstance(result, MockComponent) + + +def test_component_load_classmethod_calls_loader(): + """ + Test that Component.load() classmethod actually works end-to-end. + This verifies the fix for the bug at component.py:139-140. + + Note: This test verifies the plumbing works by checking that + the registered loader is called. Since Component already has + loaders registered, we temporarily replace one to test. + """ + # Save the original loader + original_loader = DEFAULT_REGISTRY._loaders.get((Component, MF6)) + + # Track whether the loader was called + loader_calls = [] + + def mock_loader(cls, path, format=MF6): + """Mock loader that returns an instance with no children.""" + loader_calls.append({"cls": cls, "path": path, "format": format}) + # Return an instance (simulating a loaded component) + instance = cls(name="loaded_component") + return instance + + # Temporarily replace the Component loader + DEFAULT_REGISTRY._loaders[(Component, MF6)] = mock_loader + + try: + # Call Component.load() classmethod + test_path = Path("/test/component.txt") + Component.load(test_path, format=MF6) + + # Verify the loader was called + assert len(loader_calls) == 1 + assert loader_calls[0]["cls"] is Component + assert loader_calls[0]["path"] == test_path + assert loader_calls[0]["format"] == MF6 + + finally: + # Restore the original loader + if original_loader: + DEFAULT_REGISTRY._loaders[(Component, MF6)] = original_loader + else: + del DEFAULT_REGISTRY._loaders[(Component, MF6)] + + +def test_component_load_with_children(): + """ + Test that Component.load() correctly loads children. + This ensures the fix allows accessing self.children in the classmethod. + """ + # Save original loader + original_loader = DEFAULT_REGISTRY._loaders.get((Component, MF6)) + + # Track all load calls + all_loads = [] + + def mock_loader(cls, path, format=MF6): + """Mock loader that tracks calls.""" + all_loads.append({"cls": cls, "path": str(path), "format": format}) + + # Create instance + instance = cls(name=f"loaded_{cls.__name__}") + + # Simulate children only for the parent component + if "parent" in str(path): + # Add mock children that have paths + @xattree + class ChildComponent(Component): + name: str = "child" + + def default_filename(self): + return "child.txt" + + # Attach a child with a path + child = ChildComponent(name="child1", filename="child1.txt") + # Children dict should exist from xattree + instance.children["child1"] = child + + return instance + + # Temporarily replace the Component loader + DEFAULT_REGISTRY._loaders[(Component, MF6)] = mock_loader + + try: + # Load a component with children + Component.load(Path("/test/parent.txt"), format=MF6) + + # Should have called loader for parent AND child + assert len(all_loads) == 2, f"Expected 2 loads (parent + child), got {len(all_loads)}" + + # First call should be for parent + assert "/test/parent.txt" in all_loads[0]["path"] + assert all_loads[0]["format"] == MF6 + + # Second call should be for child (with its relative path) + # Note: child path is relative to cwd, not parent's directory + assert "child1.txt" in all_loads[1]["path"] + assert all_loads[1]["format"] == MF6 + + finally: + # Restore original loader + if original_loader: + DEFAULT_REGISTRY._loaders[(Component, MF6)] = original_loader + else: + del DEFAULT_REGISTRY._loaders[(Component, MF6)] + + +def test_context_load_with_workspace(): + """ + Test that Context.load() uses workspace for loading children. + + Context components have a workspace attribute, and children should + be loaded relative to that workspace, not the current directory. + """ + import tempfile + + # Create a test Context subclass + @xattree + class TestContext(Context): + """Test context component with workspace.""" + + name: str = "test_context" + + # Save original loader + original_loader = DEFAULT_REGISTRY._loaders.get((Context, MF6)) + + # Track all load calls with their working directory + all_loads = [] + + # Create a temporary workspace directory + with tempfile.TemporaryDirectory() as tmpdir: + workspace_dir = Path(tmpdir) / "workspace" + workspace_dir.mkdir() + + def mock_loader(cls, path, format=MF6): + """Mock loader that tracks calls and current directory.""" + all_loads.append( + {"cls": cls, "path": str(path), "format": format, "cwd": str(Path.cwd())} + ) + + # Create instance + instance = cls(name=f"loaded_{cls.__name__}") + + # Set workspace for parent + if "parent" in str(path): + instance.workspace = workspace_dir + + # Add mock child + @xattree + class ChildComponent(Component): + name: str = "child" + + child = ChildComponent(name="child1", filename="child1.txt") + instance.children["child1"] = child + + return instance + + # Register loader for Context (will match TestContext via subclass) + DEFAULT_REGISTRY._loaders[(Context, MF6)] = mock_loader + # Also register for Component (for the child) + DEFAULT_REGISTRY._loaders[(Component, MF6)] = mock_loader + + try: + # Load a context component with children + TestContext.load(Path("/test/parent.txt"), format=MF6) + + # Should have called loader for parent AND child + assert len(all_loads) == 2, f"Expected 2 loads (parent + child), got {len(all_loads)}" + + # First call should be for parent + assert "/test/parent.txt" in all_loads[0]["path"] + assert all_loads[0]["format"] == MF6 + + # Second call should be for child + assert "child1.txt" in all_loads[1]["path"] + assert all_loads[1]["format"] == MF6 + + # IMPORTANT: Child should be loaded with cwd = parent's workspace + # This is the key feature of Context.load() + # Resolve both paths to handle symlinks (e.g., /private/var vs /var on macOS) + actual_cwd = Path(all_loads[1]["cwd"]).resolve() + expected_cwd = workspace_dir.resolve() + assert actual_cwd == expected_cwd, ( + f"Child should be loaded in parent's workspace {expected_cwd}, " + f"but was loaded in {actual_cwd}" + ) + + finally: + # Restore original loaders + if original_loader: + DEFAULT_REGISTRY._loaders[(Context, MF6)] = original_loader + else: + if (Context, MF6) in DEFAULT_REGISTRY._loaders: + del DEFAULT_REGISTRY._loaders[(Context, MF6)] + + # Clean up Component loader + if (Component, MF6) in DEFAULT_REGISTRY._loaders: + del DEFAULT_REGISTRY._loaders[(Component, MF6)] From 108c1f78c04826f55e0c21d4da16bc303c262ead Mon Sep 17 00:00:00 2001 From: wpbonelli Date: Mon, 24 Nov 2025 04:42:13 -0500 Subject: [PATCH 2/3] remove bot readme --- test/IO_PLUMBING_TESTS_README.md | 131 ------------------------------- 1 file changed, 131 deletions(-) delete mode 100644 test/IO_PLUMBING_TESTS_README.md diff --git a/test/IO_PLUMBING_TESTS_README.md b/test/IO_PLUMBING_TESTS_README.md deleted file mode 100644 index 20f6b1ef..00000000 --- a/test/IO_PLUMBING_TESTS_README.md +++ /dev/null @@ -1,131 +0,0 @@ -# IO Plumbing Tests for Classmethod Load - -## Overview - -This test suite (`test_io_plumbing.py`) validates the IO plumbing layer for the refactored `Component.load()` classmethod. The tests ensure that: - -1. IO descriptors work correctly in classmethod contexts -2. The registry can register and lookup loaders/writers -3. Loaders can be invoked from class methods (not just instances) -4. The plumbing layer properly supports inheritance -5. Child components are loaded recursively -6. Context components load children relative to their workspace - -## Test Coverage (16 Tests) - -### Core Descriptor Tests - -- **`test_io_descriptor_access_from_class()`**: Verifies that `_load` descriptor can be accessed from a class (without an instance) -- **`test_io_descriptor_access_from_instance()`**: Verifies that `_load` descriptor still works with instances -- **`test_io_descriptor_callable()`**: Confirms the descriptor returns a callable Loader object - -### Registry Tests - -- **`test_loader_registry_lookup()`**: Tests basic loader registration and lookup -- **`test_loader_registry_subclass_lookup()`**: Verifies that loaders registered for base classes work with subclasses -- **`test_multiple_format_registrations()`**: Tests that different loaders can be registered for different formats - -### Classmethod Integration Tests - -- **`test_classmethod_load_with_mock_loader()`**: Full integration test showing loader invocation from classmethod context -- **`test_classmethod_load_signature()`**: Validates the `load()` method signature -- **`test_classmethod_load_should_return_instance()`**: Demonstrates expected behavior (loader should return an instance) -- **`test_loader_can_be_called_directly_from_class()`**: Tests the exact pattern used in `Component.load()` -- **`test_component_load_classmethod_calls_loader()`**: Verifies that `Component.load()` correctly invokes registered loaders -- **`test_component_load_with_children()`**: Tests that children are loaded recursively - -### Context/Workspace Tests - -- **`test_context_load_with_workspace()`**: Verifies that `Context.load()` loads children relative to the parent's workspace directory - -### Writer Tests - -- **`test_writer_descriptor_requires_instance()`**: Verifies writers work correctly with instances -- **`test_registry_write_with_mock_writer()`**: Tests writer registration and invocation - -### Type Annotation Tests - -- **`test_load_return_type()`**: Validates the return type annotation of the `load()` method - -## Issues Fixed - -### ✅ Loader Signature Mismatch (flopy4/mf6/__init__.py) - -**Problem**: Loader functions had old signature without `cls` parameter: -```python -def _load_mf6(path: Path) -> Component: # ❌ Missing cls parameter -``` - -**Fixed**: Updated all loaders to accept `cls` as first parameter: -```python -def _load_mf6(cls, path: Path) -> Component: # ✅ Correct signature -def _load_json(cls, path: Path) -> Component: -def _load_toml(cls, path: Path) -> Component: -``` - -### ✅ Bug in Component.load() (flopy4/mf6/component.py:139-140) - -**Problem**: Used undefined `self` in classmethod context: -```python -@classmethod -def load(cls, path: str | PathLike, format: str = MF6) -> None: - cls._load(path, format=format) - for child in self.children.values(): # ❌ self doesn't exist -``` - -**Fixed**: Assign loader result to `self`: -```python -@classmethod -def load(cls, path: str | PathLike, format: str = MF6) -> None: - self = cls._load(path, format=format) # ✅ Get the instance - for child in self.children.values(): - child.__class__.load(child.path, format=format) -``` - -### ✅ Context.load() Workspace Support (flopy4/mf6/context.py) - -**Problem**: `Context.load()` was an instance method and didn't use workspace for children: -```python -def load(self, format=MF6): # ❌ Instance method - with cd(self.workspace): - super().load(format=format) -``` - -**Fixed**: Made it a classmethod that loads children within workspace: -```python -@classmethod -def load(cls, path, format=MF6): # ✅ Classmethod - """Load context and children relative to workspace.""" - instance = cls._load(path, format=format) - - # Load children within the workspace context - with cd(instance.workspace): - for child in instance.children.values(): - child.__class__.load(child.path, format=format) -``` - -Now child components are loaded with paths relative to the parent's workspace directory, not the current working directory. - -## Running the Tests - -```bash -pixi run -e dev pytest test/test_io_plumbing.py -v -``` - -All 16 tests should pass ✅ - -## Summary - -The IO plumbing is now fully functional for classmethod-based loading: - -1. ✅ Loader functions have correct signature with `cls` parameter -2. ✅ `Component.load()` correctly loads parent and children -3. ✅ `Context.load()` loads children relative to parent's workspace -4. ✅ Registry properly looks up loaders for classes and subclasses -5. ✅ All descriptors work in both class and instance contexts - -## Next Steps - -1. Wire up the actual loader implementations to the converter/transformer layers -2. Add integration tests with real MF6 files -3. Consider whether `load()` should return the loaded instance for user convenience From 0bde703314b3702b5a3cc425aa20b648bd10a94a Mon Sep 17 00:00:00 2001 From: wpbonelli Date: Mon, 24 Nov 2025 04:50:07 -0500 Subject: [PATCH 3/3] normalize path seps --- test/test_io_plumbing.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test_io_plumbing.py b/test/test_io_plumbing.py index 48c3b03b..ce87f7fa 100644 --- a/test/test_io_plumbing.py +++ b/test/test_io_plumbing.py @@ -381,12 +381,12 @@ def default_filename(self): assert len(all_loads) == 2, f"Expected 2 loads (parent + child), got {len(all_loads)}" # First call should be for parent - assert "/test/parent.txt" in all_loads[0]["path"] + assert "/test/parent.txt" in all_loads[0]["path"].replace("\\", "/") assert all_loads[0]["format"] == MF6 # Second call should be for child (with its relative path) # Note: child path is relative to cwd, not parent's directory - assert "child1.txt" in all_loads[1]["path"] + assert "child1.txt" in all_loads[1]["path"].replace("\\", "/") assert all_loads[1]["format"] == MF6 finally: @@ -460,11 +460,11 @@ class ChildComponent(Component): assert len(all_loads) == 2, f"Expected 2 loads (parent + child), got {len(all_loads)}" # First call should be for parent - assert "/test/parent.txt" in all_loads[0]["path"] + assert "/test/parent.txt" in all_loads[0]["path"].replace("\\", "/") assert all_loads[0]["format"] == MF6 # Second call should be for child - assert "child1.txt" in all_loads[1]["path"] + assert "child1.txt" in all_loads[1]["path"].replace("\\", "/") assert all_loads[1]["format"] == MF6 # IMPORTANT: Child should be loaded with cwd = parent's workspace