ops.Relation.load() should perhaps be strict

[dimaqq]

Context:

In [24]: class C(BaseModel):
    ...:     foo: int
    ...:

In [25]: C(foo="42")
Out[25]: C(foo=42)

In [26]: C.model_validate({"foo": "42"})
Out[26]: C(foo=42)

In [27]: C.model_validate({"foo": "42"}, strict=True)
---------------------------------------------------------------------------
ValidationError                           Traceback (most recent call last)
Cell In[27], line 1
----> 1 C.model_validate({"foo": "42"}, strict=True)

File ~/.cache/uv/archive-v0/j9AUHBx4OV-OhSDXYcYzU/lib/python3.8/site-packages/pydantic/main.py:627, in BaseModel.model_validate(cls, obj, strict, from_attributes, context)
    625 # `__tracebackhide__` tells pytest and some other tools to omit this function from tracebacks
    626 __tracebackhide__ = True
--> 627 return cls.__pydantic_validator__.validate_python(
    628     obj, strict=strict, from_attributes=from_attributes, context=context
    629 )

ValidationError: 1 validation error for C
foo
  Input should be a valid integer [type=int_type, input_value='42', input_type=str]
    For further information visit https://errors.pydantic.dev/2.10/v/int_type

Today, we use Pydantic's "magic" .model_dump in ops.Relation.save() but we instantiate the type directly in ops.Relation.load().

If we used Pydantic's "magic" .model_validate, we could pass in strict=True and avoid type coersion.

[dimaqq]

cc @tonyandrewmeyer wdyt?

[dimaqq]

From stand-up, class-level model_config is a possible workaround:

In [30]: class C(BaseModel):
    ...:     foo: int
    ...:     model_config = pydantic.ConfigDict(strict=True)

This would probably have to be set on every BaseModel-derived class (e.g. when one model includes another model, which includes yet another model, ...):

In [40]: class B(BaseModel):
    ...:     b: int
    ...:     model_config = pydantic.ConfigDict(strict=True)
    ...:

In [41]: class C(BaseModel):
    ...:     c: typing.List[B]
    ...:     model_config = pydantic.ConfigDict(strict=True)
    ...:

[james-garner-canonical]

I think it would be nice to use model_validate (if it exists) in Relation.load. But I'm not sure that this is possible, since load accepts arbitrary *args and **kwargs to pass to the class constructor.

If we did want to go the model_validate route, I guess it could be an error to pass extra arguments if the class has model_validate.

Or perhaps kwargs could be splatted into the relation data (though args are still problematic -- though I guess we could infer their names from the constructor signature to splat them in as well ... which is getting pretty 'dark arts' ...).

[tonyandrewmeyer]

cc @tonyandrewmeyer wdyt?

My concerns with this are:

• It's a backwards incompatible change, which should rule it out.
• I think logically the decisions about how to validate and/or convert data belong in the class, not in ops.
• Not every class will have a model_validate method, so we would have to put in more Pydantic-specific code, and I'd rather we had less rather than more.
• Similarly, I think we should keep the behaviour as similar as possible regardless of the type of object. We can do that by leaving these choices to the classes, but we can't if we start hooking up unique behaviour for specific dataclass/model libraries.
• Right now, users have flexibility - they can choose to enforce strict validation with a simple model config option, or they can choose to not do so. As far as I can see, if we made this change, we'd remove that flexibility and force strict validation. I don't know if there's evidence to back that up. There's also more flexibility in how it's used, the whole model with config, or per-field with the metaclass or field option.
• The arbitrary args/kwargs that James mentions, which are there to support existing use cases where charms create data class objects that are a combination of relation data and other data (for example, that extract secrets by having access to the model).
• If you don't like the Relation.load behaviour and you're using Pydantic, it's pretty trivial to just use model_validate yourself if that's what you want. I don't think I've seen any justification for baking that into ops.

I have no objections to using strict=True in model config in our Pydantic examples in documentation, or otherwise recommending using it if you're using Pydantic.

[dimaqq]

That's fair push-back.

I would like to counter the "flexibility" argument: we're supposed to be reasonably opinionated. Senior leadership at some point opined "don't make it generic, stick to Pydantic". To extrapolate from that, we should, arguably, enforce a good set of defaults in:

• obj.model_dump(mode='json', by_alias=True, exclude_defaults=False) in Relation.save() which we do
• cls.model_validate(self.data[src], strict=True, ...) in Relation.load() which today we don't

[tonyandrewmeyer]

Senior leadership at some point opined "don't make it generic, stick to Pydantic".

That's a bit out of context. That discussion was focused on making Pydantic a dependency of ops in a mistaken belief that it would somehow make packing charms easier because it would somehow remove the need for rust. It definitely was not an informed discussion about whether or not Pydantic should always be used.

The Charm Tech position is better described as "we'll support multiple approaches, including standard library only, but recommend Pydantic in our documentation and examples".

To extrapolate from that, we should, arguably, enforce a good set of defaults in:

• cls.model_validate(self.data[src], strict=True, ...) in Relation.load() which today we don't

I also think it's an extension to go from recommending (or anything strong) Pydantic to saying how the classes should be defined and how validation should be done. It seems reasonable to discuss that and certainly to have stronger guidance on it (docs, examples, specs, etc), but I don't believe it is something that arises from the earlier discussions about whether Pydantic is a good choice or not.

What about a validate argument for loading, like rel.load(MyModel, validate=True)? That would address my concerns about backwards compatibility, and arbitrary args/kwargs, and continue to offer flexibility if charms want it for validation, but we could push always validating at this point if we feel that's the right move. We could default to False, but warn with a default value and switch the default in a major version bump.

However, I still would like to see examples of where this would benefit real charms - like cases where they are calling model_validate themselves and would benefit from removing those lines of code, or where there's model config that's enabling verification but not doing anything else and again the code would be cleaner.

[james-garner-canonical]

What about a validate argument for loading, like rel.load(MyModel, validate=True)? That would address my concerns about backwards compatibility, and arbitrary args/kwargs, and continue to offer flexibility if charms want it for validation, but we could push always validating at this point if we feel that's the right move. We could default to False, but warn with a default value and switch the default in a major version bump.

I think gating this behaviour change behind a flag is the right thing to do for backwards compatibility. We can push users towards explicitly using validate= in our docs with the warning that the default will change in future. This gets around both the backwards incompatible change in behaviour, and the backwards incompatible change in accepting *args, **kwargs. (Passing extra positional arguments should be an error with validate=True and a Pydantic model.)

However, technically adding a new keyword argument is also a breaking change, since previously validate=True would have meant that validate was passed to the user's class in kwargs. Perhaps it could be a property of Relation instead.

[dimaqq]

Let's punt the discussion to next major release.