Skip to content

Improve bindings for configuration types #35

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.

Sign up for GitHub

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

Open
wdeconinck wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Improve bindings for configuration types #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3644b68
Improve config
wdeconinck Feb 11, 2026
df967b1
Version 0.41.1.dev5
wdeconinck Jul 1, 2026
87c8da3
Fix nit on using nb::str
wdeconinck Aug 28, 2026
3cf4e1f
Remove unneeded overload
wdeconinck Aug 28, 2026
917a370
Forward compatibility to handle vector<bool> correctly.
wdeconinck Aug 28, 2026
ed6afa5
Add unit-test that asserts Metadata follows the mapping protocol
wdeconinck Aug 28, 2026
2fd129f
More defensive path handling
wdeconinck Aug 29, 2026
59cecf5
Change error types to the mapping protocol expectations
wdeconinck Aug 29, 2026
6ed96da
Fix handling of None value
wdeconinck Aug 29, 2026
48479a9
Remove make_Config and inline it at callsite
wdeconinck Aug 29, 2026
4b7dad2
Be more explicit that both yaml and json are parsed by the same parser
wdeconinck Aug 29, 2026
e8a9101
Replace atlas4py.Config.from_kwargs with constructor
wdeconinck Aug 29, 2026
e88c54d
Make eckit.Configuration and eckit.LocalConfiguration private
wdeconinck Sep 1, 2026
a7a2d6b
Address #39 partially, only for the Config related classes
wdeconinck Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ dependencies = []

description = 'Python bindings for Atlas: a ECMWF library for parallel data-structures'
name = 'atlas4py'
version = '0.41.1.dev4' # <major>.<minor>.<patch>.dev<dev> : <atlas.major>.<atlas.minor>.<atlas.patch>.dev<atlas4py.dev>
version = '0.41.1.dev5' # <major>.<minor>.<patch>.dev<dev> : <atlas.major>.<atlas.minor>.<atlas.patch>.dev<atlas4py.dev>
license = {text = "Apache License 2.0"}
readme = {file = 'README.md', content-type = 'text/markdown'}
authors = [{email = 'willem.deconinck@ecmwf.int'}, {name = 'Willem Deconinck'}]
Expand Down
5 changes: 4 additions & 1 deletion src/atlas4py/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ endif()

### Python bindings module atlas4py

nanobind_add_module(_atlas4py _atlas4py.cpp)
nanobind_add_module(_atlas4py
_atlas4py.cpp
_atlas4py_Config.cpp
)
target_link_libraries(_atlas4py PUBLIC atlas)
target_compile_definitions(_atlas4py PRIVATE ATLAS4PY_VERSION_STRING=${PROJECT_VERSION_FULL})

Expand Down
147 changes: 10 additions & 137 deletions src/atlas4py/_atlas4py.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ namespace pluto {
#include "eckit/value/Value.h"
#include "eckit/config/Configuration.h"

#include "_atlas4py_Config.hpp"

namespace nb = ::nanobind;

using namespace atlas;
Expand Down Expand Up @@ -151,77 +153,6 @@ void atlasInitialise() {
// the atlas library and deleting it may cause issues.
}

nb::object toPyObject( eckit::Configuration const& v );
nb::object toPyObject( eckit::Configuration const& v, std::string const& key );

nb::object toPyObject(bool v) {
return nb::bool_(v);
}
nb::object toPyObject(long v) {
return nb::int_(v);
}
nb::object toPyObject(double v) {
return nb::float_(v);
}
nb::object toPyObject(std::string const& v) {
return nb::str(v.c_str());
}
template <typename T>
nb::object toPyObject( std::vector<T> const& v ) {
nb::list ret;
for ( auto const& val : v ) {
ret.append( toPyObject( val ) );
}
return ret;
}

nb::object toPyObject( eckit::Configuration const& v, std::string const& key ) {
if ( v.isSubConfiguration ( key ) ) {
return toPyObject( v.getSubConfiguration( key ) );
}
else if (v.isBoolean( key )) {
return toPyObject( v.getBool( key ) );
}
else if (v.isIntegral( key )) {
return toPyObject( v.getLong( key ) );
}
else if (v.isFloatingPoint( key )) {
return toPyObject( v.getDouble( key ) );
}
else if (v.isString( key )) {
return toPyObject( v.getString( key ) );
}
else if (v.isSubConfigurationList( key )) {
std::vector<eckit::LocalConfiguration> subconfigs = v.getSubConfigurations( key );
return toPyObject( subconfigs );
}
else if (v.isIntegralList( key )) {
std::vector<long> values = v.getLongVector( key );
return toPyObject( values );
}
else if (v.isFloatingPointList( key )) {
std::vector<double> values = v.getDoubleVector( key );
return toPyObject( values );
}
else if (v.isStringList( key )) {
std::vector<std::string> values = v.getStringVector( key );
return toPyObject( values );
}
else if (v.isBooleanList( key )) {
throw std::out_of_range( "boolean lists not supported for key " + key );
}
else {
throw std::out_of_range( "type of value unsupported for key " + key );
}
}

nb::object toPyObject( eckit::Configuration const& v ) {
nb::dict ret;
for ( auto const& key : v.keys()) {
ret[ key.c_str() ] = toPyObject( v, key );
}
return ret;
}

int get_nb_device_type( const void* ptr ) {
if (pluto::is_pinned(ptr)) {
Expand Down Expand Up @@ -293,6 +224,8 @@ NB_MODULE( _atlas4py, m ) {
.def("_finalise", atlas::finalise);
m.attr("__version__") = STRINGIFY(ATLAS4PY_VERSION_STRING);

atlas4py::bind_Config( m );

nb::class_<PointLonLat>( m, "PointLonLat" )
.def( nb::init<double,double>(), "lon"_a, "lat"_a )
.def_prop_ro( "lon", nb::overload_cast<>( &PointLonLat::lon, nb::const_ ) )
Expand All @@ -310,7 +243,7 @@ NB_MODULE( _atlas4py, m ) {

nb::class_<Projection>( m, "Projection" )
.def( "__repr__", []( Projection const& p ) {
return "_atlas4py.Projection("_s + nb::str( toPyObject( p.spec() ) ) + ")"_s;
return "_atlas4py.Projection("_s + nb::str( atlas4py::make_object( p.spec() ) ) + ")"_s;
} );

nb::class_<Domain>( m, "Domain" )
Expand All @@ -319,7 +252,7 @@ NB_MODULE( _atlas4py, m ) {
.def_prop_ro( "units", &Domain::units )
.def( "__repr__", []( Domain const& d ) {
if (d) {
return nb::str("_atlas4py.Domain("_s + nb::str( toPyObject( d.spec() ) ) + ")"_s);
return nb::str("_atlas4py.Domain("_s + nb::str( atlas4py::make_object( d.spec() ) ) + ")"_s);
}
return nb::str("_atlas4py.Domain()"_s);
} );
Expand All @@ -334,13 +267,13 @@ NB_MODULE( _atlas4py, m ) {
.def_prop_ro( "projection", &Grid::projection )
.def_prop_ro( "domain", &Grid::domain )
.def( "__repr__",
[]( Grid const& g ) { return "_atlas4py.Grid("_s + nb::str( toPyObject( g.spec() ) ) + ")"_s; } );
[]( Grid const& g ) { return "_atlas4py.Grid("_s + nb::str( atlas4py::make_object( g.spec() ) ) + ")"_s; } );

nb::class_<grid::Spacing>( m, "Spacing" )
.def( "__len__", &grid::Spacing::size )
.def( "__getitem__", &grid::Spacing::operator[])
.def( "__repr__", []( grid::Spacing const& spacing ) {
return "_atlas4py.Spacing("_s + nb::str( toPyObject( spacing.spec() ) ) + ")"_s;
return "_atlas4py.Spacing("_s + nb::str( atlas4py::make_object( spacing.spec() ) ) + ")"_s;
} );
nb::class_<grid::LinearSpacing, grid::Spacing>( m, "LinearSpacing" )
.def( nb::init<double,double,long,bool>(), "start"_a, "stop"_a, "N"_a, "endpoint_included"_a = true );
Expand Down Expand Up @@ -373,41 +306,6 @@ NB_MODULE( _atlas4py, m ) {
.def_prop_ro( "regular", &StructuredGrid::regular )
.def_prop_ro( "periodic", &StructuredGrid::periodic );

nb::class_<eckit::Configuration>( m, "eckit.Configuration" );

nb::class_<eckit::LocalConfiguration, eckit::Configuration>( m, "eckit.LocalConfiguration" );

// TODO This is a duplicate of metadata below (because same base class)
nb::class_<util::Config, eckit::LocalConfiguration>( m, "Config" )
.def( nb::init() )
.def( "__setitem__",
[]( util::Config& config, std::string const& key, nb::object value ) {
if ( nb::isinstance<nb::bool_>( value ) )
config.set( key, nb::cast<bool>( value ) );
else if ( nb::isinstance<nb::int_>( value ) )
config.set( key, nb::cast<long long>( value ) );
else if ( nb::isinstance<nb::float_>( value ) )
config.set( key, nb::cast<double>( value ) );
else if ( nb::isinstance<nb::str>( value ) )
config.set( key, nb::cast<std::string>( value ) );
else
throw std::out_of_range( "type of value unsupported" );
} )
.def( "__getitem__",
[]( util::Config& config, std::string const& key ) -> nb::object {
if ( !config.has( key ) )
throw std::out_of_range( "key <" + key + "> could not be found" );

// TODO: We have to query metadata.get() even though this should
// not be done (see comment in Config::get). We cannot
// avoid this right now because otherwise we cannot query
// the type of the underlying data.
return toPyObject( config, key );
} )
.def( "__repr__", []( util::Config const& config ) {
return "_atlas4py.Config("_s + nb::str( toPyObject( config ) ) + ")"_s;
} );

nb::class_<Field>( m, "Field" )
.def_prop_ro( "name", &Field::name )
.def_prop_ro( "strides", &Field::strides )
Expand Down Expand Up @@ -574,34 +472,9 @@ NB_MODULE( _atlas4py, m ) {
.def_prop_ro( "cells", &functionspace::CellColumns::cells )
.def_prop_ro( "valid", &functionspace::CellColumns::valid );

nb::class_<util::Metadata>( m, "Metadata" )
.def_prop_ro( "keys", &util::Metadata::keys )
.def( "__setitem__",
[]( util::Metadata& metadata, std::string const& key, nb::object value ) {
if ( nb::isinstance<nb::bool_>( value ) )
metadata.set( key, nb::cast<bool>(value) );
else if ( nb::isinstance<nb::int_>( value ) )
metadata.set( key, nb::cast<long long>(value) );
else if ( nb::isinstance<nb::float_>( value ) )
metadata.set( key, nb::cast<double>(value) );
else if ( nb::isinstance<nb::str>( value ) )
metadata.set( key, nb::cast<std::string>(value) );
else
throw std::out_of_range( "type of value unsupported" );
} )
.def( "__getitem__",
[]( util::Metadata& metadata, std::string const& key ) -> nb::object {
if ( !metadata.has( key ) )
throw std::out_of_range( "key <" + key + "> could not be found" );

// TODO: We have to query metadata.get() even though this should
// not be done (see comment in Config::get). We cannot
// avoid this right now because otherwise we cannot query
// the type of the underlying data.
return toPyObject( metadata, key );
} )
nb::class_<util::Metadata, eckit::LocalConfiguration>( m, "Metadata" )

@tehrengruber-ai tehrengruber-ai Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@wdeconinck wdeconinck Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

.def( "__repr__", []( util::Metadata const& metadata ) {
return "_atlas4py.Metadata("_s + nb::str( toPyObject( metadata ) ) + ")"_s;
return "atlas4py.Metadata("_s + nb::str( atlas4py::make_object( metadata ) ) + ")"_s;
} );

nb::class_<mesh::Nodes::Topology> topology( m, "Topology" );
Expand Down
Loading
Loading