Improve bindings for configuration types by wdeconinck · Pull Request #35 · GridTools/atlas4py

wdeconinck · 2026-04-29T20:59:04Z

This pull request refactors and extends the Python bindings for configuration objects in atlas4py, improving maintainability and usability. The main changes involve extracting the configuration binding logic into a new dedicated source file, enhancing the Python interface for configuration objects, and updating tests to cover the new features.

Refactoring and modularization:

Extracted all logic related to Python bindings for configuration objects (eckit::Configuration, eckit::LocalConfiguration, atlas::util::Config) into new files: _atlas4py_Config.cpp and _atlas4py_Config.hppThis significantly improves code organization and maintainability.

Enhancements to Python bindings:

Replaced previous custom Python conversion and binding code for configuration objects with a more robust and feature-rich implementation in the new module. The new bindings provide a consistent interface for getting, setting, and representing configuration objects, and add static constructors for creating Config objects from keyword arguments, YAML strings, and files.
Updated the __repr__ methods for several types (Projection, Domain, Grid, Spacing, Metadata) to use the new atlas4py::make_object function for consistent and improved string representations.

Testing improvements:

Added new tests to tests/test_bindings.py to verify the new Config constructors (from_kwargs, from_yaml, from_file) and their correct behavior, including nested configuration and type handling.

Copilot

Pull request overview

This PR refactors the atlas4py Python bindings for configuration-related types by extracting config binding logic into a dedicated module and expanding the Python-facing Config API (including YAML/file constructors), while updating string representations to use the new config-to-Python conversion helper.

Changes:

Split configuration binding/conversion logic into _atlas4py_Config.{hpp,cpp} and register it from _atlas4py.cpp.
Expanded Python bindings for eckit::Configuration / eckit::LocalConfiguration / atlas::util::Config (mapping protocol, constructors from kwargs/YAML/file).
Added pytest coverage for new Config behaviors and bumped the package dev version.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`tests/test_bindings.py`	Adds tests for new `Config` constructors and mapping/contains semantics.
`src/atlas4py/CMakeLists.txt`	Ensures the new config binding translation unit is compiled into `_atlas4py`.
`src/atlas4py/_atlas4py.cpp`	Wires in `bind_Config()` and updates several `__repr__` paths to use `make_object()`.
`src/atlas4py/_atlas4py_Config.hpp`	Declares the new config binding/conversion API surface used by `_atlas4py.cpp`.
`src/atlas4py/_atlas4py_Config.cpp`	Implements the new binding/conversion logic and `Config` constructors.
`pyproject.toml`	Bumps `atlas4py` version from `0.41.1.dev4` to `0.41.1.dev5`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wdeconinck · 2026-07-01T10:39:11Z

Hi @tehrengruber this is the following PR in line, now ready for you to review :)

tehrengruber-ai

Reviewed the new _atlas4py_Config.{cpp,hpp} against the eckit and atlas sources. The refactor reads well and the mapping protocol is a real improvement — most of what's below is about round-trip fidelity and the exception types that the new protocol implies.

Highest value, in order:

Missing keys raise IndexError, not KeyError — now part of the mapping contract.
c["x"] = [True, False] reads back as [1, 0], and the new test can't catch it because 1 == True.
A null value in a YAML/JSON file makes repr() of the whole config raise.
Metadata.keys silently changed from a property to a method.

I also checked a few things that turned out fine, for the record: the __iter__ lifetime is safe (nb::cast materializes an owning list that the iterator holds a reference to, so no dangling); the isBoolean/isIntegral probe order is not order-sensitive since BoolContent doesn't override isNumber(); the str/sequence/mapping dispatch order in config_set is correct; every LocalConfiguration::set overload used exists; and key insertion order is preserved (Value::makeOrderedMap), so the order-asserting tests aren't flaky.

Separately, the __repr__ strings name a private module (_atlas4py.) and aren't evaluable — that's pre-existing on master rather than anything this PR introduces, so I've filed it as #39 instead of asking for it here. The one point of contact is that this PR is the first to put eckit.Configuration into a user-visible string, and that name isn't reachable by attribute access.

tehrengruber-ai · 2026-08-27T18:37:04Z

+        .def( nb::init() )
+        .def_static( "from_kwargs", []( nb::kwargs kwargs ) {
+            return make_Config(kwargs);
+        } )


Could from_kwargs just be the regular constructor? Config(option1="value1", option2=42) reads more naturally than Config.from_kwargs(...), and kwargs are keyword-only so there's no ambiguity with the other factories. It would also subsume the bare nb::init() above, since an empty kwargs gives an empty config.

The module already has this idiom, e.g. _atlas4py.cpp:456:

.def("__init__", [](functionspace::EdgeColumns *t, const Mesh& m, int halo) { new (t) functionspace::EdgeColumns(m, util::Config()("halo", halo)); }, "mesh"_a, "halo"_a = 0)

so here it would be:

.def( "__init__", []( atlas::util::Config* t, nb::kwargs kwargs ) { new (t) atlas::util::Config( make_Config( kwargs ) ); } )

These four def_statics are the only from_* factories in the extension, so it's a pattern that shows up nowhere else. from_yaml/from_json/from_file do need to stay static — they all take a positional string and a constructor couldn't tell YAML source from a path — but from_kwargs doesn't have that problem. It would also give make_Config (exported in _atlas4py_Config.hpp) a caller that isn't three lines below it.

Only cost is updating the new tests that use from_kwargs; since it's unreleased there's nothing external to break.

Fixed with e8a9101

tehrengruber-ai · 2026-08-27T18:37:04Z

+        .def_static( "from_file", []( const nb::object path, std::string const& format ) {
+            // path accepts string but also path-like objects (e.g. pathlib.Path)
+            if ( !format.empty() ) {
+                auto format_lowercase = to_lowercase(format);
+                if (format_lowercase != "yaml" && format_lowercase != "json") {
+                    throw std::runtime_error("Only 'yaml' or 'json' format is supported");
+                }
+            }
+            return atlas::util::Config( eckit::PathName{nb::cast<std::string>( nb::str(path) )} );
+        }, "path"_a, "format"_a = "" )


Following up on the earlier thread about format rather than reopening it — the part that still seems worth changing isn't that JSON fails to parse (it doesn't; YAML 1.2 is a superset of JSON, so both load fine through eckit::YAMLParser), it's that the parameter can't detect a mismatch:

Config.from_file("config.yaml", format="json") # succeeds, parses YAML

Since "yaml" and "json" both take the identical code path, no value of this argument can change any outcome — the check only rejects misspellings. A user who passes format= defensively gets no protection while reasonably assuming they do.

It also can't be honored later without upstream work: Config::Config(std::istream&, const std::string&) leaves its format parameter unnamed and always calls yaml_from_stream, and Config(const eckit::PathName&) always goes through yaml_from_path (atlas/util/Config.cc:39, 58-60). eckit does have a real eckit::JSONParser (eckit/parser/JSONParser.h), but wiring it up here isn't free either, since LocalConfiguration(const eckit::Value&) is protected (eckit/config/LocalConfiguration.h:94, 98) and would need a small derived shim.

So I'd suggest just dropping the parameter. If you'd rather keep it as a guard, a docstring noting that both formats are parsed by the YAML parser and that the argument does not select a parser or validate the file would remove the surprise — and std::runtime_error on line 254 would be better as nb::value_error, so a bad format raises ValueError rather than RuntimeError.

Minor, on line 257: nb::cast<std::string>( nb::str(path) ) stringifies anything, so from_file(b"/x") tries to open a file literally named b'/x'. nb::module_::import_("os").attr("fspath")(path) gives proper path-like handling and a TypeError for junk.

Fixed with 2fd129f

tehrengruber-ai · 2026-08-27T18:37:04Z

+        .def( "__getitem__",
+              []( eckit::Configuration const& config, std::string const& key ) -> nb::object {
+                  if ( !config.has( key ) )
+                      throw std::out_of_range( "key <" + key + "> could not be found" );


std::out_of_range is translated by nanobind to IndexError, so a missing key raises IndexError rather than KeyError:

try: config["missing"] except KeyError: # never fires ...

That was already the case on master, but this PR is what turns Configuration into a mapping (keys, __iter__, __len__, __contains__), so the exception type is now part of a contract users will reasonably expect to behave like dict. dict(config) and any collections.abc.Mapping-style code will be surprised by it.

nanobind has the right types built in — nb::key_error here, and nb::type_error for the unsupported-value throws in config_set (lines 163 and 173) and in _toPyObject (line 99), which currently surface as IndexError too.

Fixed with 59cecf5

tehrengruber-ai · 2026-08-27T18:37:04Z

+        if (nb::isinstance<nb::bool_>(elem)) {
+            std::vector<int> vec;
+            for (size_t i = 0; i < n; ++i) vec.push_back( nb::cast<bool>(seq[i]) ? 1 : 0 );
+            config.set(key, vec);


Booleans set from Python don't round-trip as booleans:

c = Config() c["flags"] = [True, False, True] c["flags"] # [1, 0, 1] — ints, not bools

The vector<int> here is unavoidable (LocalConfiguration has no set(name, vector<bool>) overload — eckit/config/LocalConfiguration.h:59-65), but the consequence is that Configuration::isBooleanList tests firstElement.isBool() (eckit/config/Configuration.cc:522-533), which is false for the stored NumberContent, so read-back takes the isIntegralList branch.

The result is that the set path and the parse path disagree for the same data: from_yaml("flags: [true, false]") gives real bools, c["flags"] = [True, False] gives ints.

Worth noting the new test doesn't catch this — tests/test_bindings.py:189 asserts c["bool_list"] == [True, False, True], which passes for [1, 0, 1] since 1 == True in Python. The YAML round-trip tests added in this same PR do assert isinstance(flag, bool); the set-path test should do the same. A real fix needs an eckit overload upstream, so pinning and documenting the asymmetry may be the practical option for now.

The real required fix will only be possible with the eckit overload upstream here: ecmwf/eckit#330

This is truly fixed with 917a370 provided that the above eckit PR gets merged and is used. Before that this is still backwards compatible.

tehrengruber-ai · 2026-08-27T18:37:04Z

+    else {
+        throw std::out_of_range( "type of value unsupported for key " + key );


A null in the source config isn't handled by any of the probes above, so it reaches this throw — which takes out repr() of the entire config, not just the one key:

c = Config.from_yaml("a: null") "a" in c # True c["a"] # IndexError dict(c) # IndexError print(c) # IndexError <- __repr__ goes through make_object

null is common enough in config files that hitting it via print() would be a confusing first encounter. eckit already exposes the predicate — Configuration::isNull (eckit/config/Configuration.cc:503-507) — so an isNull branch returning nb::none() before the final else covers it.

The set direction (c["a"] = None) has no LocalConfiguration overload, so continuing to reject it is fine — ideally as TypeError, per the other comment on exception types.

Fixed with 6ed96da

tehrengruber-ai · 2026-08-27T18:37:04Z

-                  // the type of the underlying data.
-                  return toPyObject( metadata, key );
-              } )
+    nb::class_<util::Metadata, eckit::LocalConfiguration>( m, "Metadata" )


Inheriting from eckit::LocalConfiguration here is a nice simplification — it removes the duplicated __getitem__/__setitem__ and makes dict(metadata) work. But it silently changes keys from a property to a method: master had .def_prop_ro( "keys", &util::Metadata::keys ), and Configuration::keys is bound as .def("keys", ...).

field.metadata.keys # was: ['name', ...] now: <bound method>

No error, just a wrong value — truthy, and it won't fail until something tries to use it as a list. Worth at least a changelog note, or a transitional keys property if you want to avoid the break. Config.keys was already a method, so only Metadata is affected.

Also note there's no test covering Metadata at all, which is why this didn't show up.

I think it is more important to change the API for metadata to use keys() function as it then follows the python mapping protocol.
I'm hoping this will not have any adverse effects to users at the moment, and be understood as an API improvement.
So my advice is: Don't fix.

tehrengruber-ai · 2026-08-27T18:37:04Z

+template <>
+nb::object _toPyObject( std::vector<bool> const& v ) {
+    nb::list ret;
+    for ( auto const& val : v ) {
+        ret.append( _toPyObject( val ) );
+    }
+    return ret;
+}


Nit: this specialization looks redundant — the primary template above handles std::vector<bool> correctly. Iterating a const vector<bool>& yields bool values (via const_reference, which is plain bool), so _toPyObject(bool) is selected and the body is identical.

Fixed with 3cf4e1f

tehrengruber-ai · 2026-08-27T18:37:04Z

+    return nb::float_(v);
+}
+nb::object _toPyObject(std::string const& v) {
+    return nb::str(v.c_str());


Nit: nb::str(v.c_str()) stops at the first embedded NUL. nb::str(v.c_str(), v.size()) is exact, and the length is already known here.

Fixed with 87c8da3

tehrengruber-ai · 2026-08-27T18:37:04Z

+#include "eckit/config/Configuration.h"
+#include "atlas/util/Config.h"
+
+namespace nb = ::nanobind;


Nit: this alias is at global scope in a header, so every translation unit that includes _atlas4py_Config.hpp gets nb injected whether it wants it or not. Moving it inside namespace atlas4py (or spelling nanobind:: out in the three declarations below) keeps it contained.

This is really harmless in the context of atlas4py, and allows for consistent naming in headers and cpp files. The C++ code is internal, and using nb as alias is common practice.

tehrengruber

I posted an LLM review, many comments look meaningful. Also upvoting the Config(...) instead of Config.from_kwargs style.

tehrengruber · 2026-08-27T13:02:21Z

+     nb::class_<atlas::util::Config, eckit::LocalConfiguration>( m, "Config" )
+        .def( nb::init() )
+        .def_static( "from_kwargs", []( nb::kwargs kwargs ) {
+            return make_Config(kwargs);


Feels like the content of make_Config could just be put here instead of wrapping it.

Fixed with 48479a9

wdeconinck · 2026-08-29T11:17:27Z

Thanks @tehrengruber I have addressed all issues, except mainly the one of metadata now using keys() as function rather than keys as property. I think this is not a big problem as this use is not widespread and hence metadata can also follow the mapping protocol better.

wdeconinck force-pushed the nanobind branch from 0be4c99 to 4db3aa3 Compare May 4, 2026 14:38

wdeconinck force-pushed the improve-config branch from 9528497 to 1190242 Compare May 4, 2026 14:58

wdeconinck force-pushed the nanobind branch from dfd33d1 to 1f4b632 Compare May 4, 2026 15:02

wdeconinck force-pushed the improve-config branch from 1190242 to b566b44 Compare May 4, 2026 15:02

wdeconinck force-pushed the nanobind branch from 1f4b632 to 503f6d4 Compare May 4, 2026 15:41

wdeconinck force-pushed the improve-config branch from b566b44 to ff588ca Compare May 4, 2026 15:42

wdeconinck force-pushed the nanobind branch from 503f6d4 to 7f503fe Compare May 5, 2026 10:58

wdeconinck force-pushed the improve-config branch from 7657847 to 5e014ca Compare May 5, 2026 10:58

wdeconinck force-pushed the nanobind branch from 4c4ee65 to 4732f8d Compare June 29, 2026 08:36

Base automatically changed from nanobind to master July 1, 2026 08:36

wdeconinck force-pushed the improve-config branch from 5e014ca to df7d464 Compare July 1, 2026 08:41

wdeconinck requested a review from Copilot July 1, 2026 08:44

Copilot started reviewing on behalf of wdeconinck July 1, 2026 08:48 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

wdeconinck force-pushed the improve-config branch from 851aa3c to ed6806c Compare July 1, 2026 09:57

wdeconinck marked this pull request as ready for review July 1, 2026 10:38

wdeconinck requested a review from tehrengruber July 1, 2026 10:38

wdeconinck added 2 commits July 7, 2026 20:52

Improve config

3644b68

Version 0.41.1.dev5

df967b1

wdeconinck force-pushed the improve-config branch from ed6806c to df967b1 Compare July 7, 2026 18:52

tehrengruber-ai mentioned this pull request Aug 27, 2026

__repr__ strings name a private module and are not evaluable #39

Open

tehrengruber-ai reviewed Aug 27, 2026

View reviewed changes

tehrengruber reviewed Aug 27, 2026

View reviewed changes

wdeconinck added 6 commits August 28, 2026 16:21

Fix nit on using nb::str

87c8da3

Remove unneeded overload

3cf4e1f

Forward compatibility to handle vector<bool> correctly.

917a370

Add unit-test that asserts Metadata follows the mapping protocol

ed6afa5

More defensive path handling

2fd129f

Change error types to the mapping protocol expectations

59cecf5

wdeconinck added 4 commits August 29, 2026 11:02

Fix handling of None value

6ed96da

Remove make_Config and inline it at callsite

48479a9

Be more explicit that both yaml and json are parsed by the same parser

4b7dad2

Replace atlas4py.Config.from_kwargs with constructor

e8a9101

wdeconinck added 2 commits September 1, 2026 20:49

Make eckit.Configuration and eckit.LocalConfiguration private

e88c54d

Address #39 partially, only for the Config related classes

a7a2d6b

		else {
		throw std::out_of_range( "type of value unsupported for key " + key );

Uh oh!

Conversation

wdeconinck commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wdeconinck commented Jul 1, 2026

Uh oh!

tehrengruber-ai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdeconinck Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdeconinck Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tehrengruber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdeconinck commented Aug 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wdeconinck commented Apr 29, 2026 •

edited

Loading

wdeconinck Aug 28, 2026 •

edited

Loading

wdeconinck Aug 28, 2026 •

edited

Loading