Skip to content

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

Description

@tehrengruber-ai

__repr__ strings name modules that don't exist and can't be evaluated.

The convention in _atlas4py.cpp is to prefix reprs with _atlas4py. and print a spec dict, e.g. (all line refs against master, a8b3365):

type repr ref
Projection _atlas4py.Projection({...}) _atlas4py.cpp:312
Domain _atlas4py.Domain({...}) _atlas4py.cpp:320
Grid _atlas4py.Grid({...}) _atlas4py.cpp:336
Spacing _atlas4py.Spacing({...}) _atlas4py.cpp:342
Config _atlas4py.Config({...}) _atlas4py.cpp:407
Metadata _atlas4py.Metadata({...}) _atlas4py.cpp:603
Gmsh _atlas4py.output.Gmsh() _atlas4py.cpp:633
PointLonLat _atlas4py.PointLonLat(lon=..., lat=...) _atlas4py.cpp:300
PointXY _atlas4py.PointXY(x=..., y=...) _atlas4py.cpp:307

Ideally a repr should be a valid expression that reconstructs the object, or — where that isn't possible — should follow a convention that makes clear it isn't one. Currently neither holds, for three separate reasons.

1. The _atlas4py. prefix names a private module

__init__.py does from ._atlas4py import *, so the name a user actually has is atlas4py.Config. _atlas4py.Config(...) only resolves if you have separately imported the private extension module, which nothing tells you to do. The prefix should be atlas4py. (or omitted).

2. _atlas4py.output.Gmsh() names nothing at all

The class is registered as plain "Gmsh" (_atlas4py.cpp:626), so there is no output submodule and no _atlas4py.output anywhere. The correct expression is atlas4py.Gmsh(path).

3. eckit.Configuration is not reachable by attribute access

_atlas4py.cpp:376,378 pass dotted names to nb::class_:

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

nanobind builds the full name as "_atlas4py" + "." + "eckit.Configuration" (nb_type.cpp:1353) and then splits it at the last dot (nb_type.cpp:929), so the class ends up with __module__ == "_atlas4py.eckit" — a module that does not exist — and __qualname__ == "Configuration". It is registered with setattr(scope, "eckit.Configuration", ...) (nb_type.cpp:1661), i.e. the module dict key literally contains a dot, so:

atlas4py.eckit.Configuration            # AttributeError
getattr(atlas4py, "eckit.Configuration")  # the only way to reach it

Since the key doesn't start with an underscore, import * also copies that unusable name into the atlas4py namespace. Either give these classes plain names (Configuration, LocalConfiguration), hide them (_Configuration), or create a real submodule with m.def_submodule("eckit").

This one matters now because #35 is the first change to put these names into user-visible strings (_atlas4py.eckit.Configuration(...)); on master the two classes are bare registrations with no methods.

4. The dict payload isn't accepted by any constructor

Even with the prefix fixed, none of the spec-dict reprs round-trip:

  • Config({'a': 1})TypeError; Config only has nb::init().
  • Projection, Domain, Spacing, Metadata have no public constructor at all.
  • Grid does have nb::init<const std::string&> (_atlas4py.cpp:330), but the repr prints the spec dict rather than the name, so Grid(...) still doesn't work.

PointLonLat / PointXY are the only two that would evaluate correctly today, once the module prefix is corrected.

Proposal

Pick one of two forms per type and apply it consistently:

  • Constructible types get an eval-able repr using the public path: atlas4py.Grid('O32') (from g.name()), atlas4py.Gmsh('out.msh'), atlas4py.PointXY(x=..., y=...). For Config, a kwargs form — atlas4py.Config(a=1, nested={'x': 2}) — round-trips exactly if kwargs construction is reachable from __init__ rather than only from the Config.from_kwargs static added in Improve bindings for configuration types #35.
  • Non-constructible types (Projection, Domain, Spacing, Metadata, and the eckit bases) use the standard non-eval form, <atlas4py.Metadata {...}>. The angle brackets are the conventional signal that the string is descriptive rather than an expression, which is more useful than a string that looks constructible and isn't.

A few doctest-style assertions (eval(repr(x)) == x for the first group, a prefix check for the second) would keep this from drifting again.

Note that apart from point 3 this is all pre-existing on master#35 only renames toPyObject to atlas4py::make_object inside the repr bodies — so this is a separate cleanup rather than something to fold into that PR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions