IR: Better tuple autocasting via pydantic field_validators #476
Conversation
Instead of using `model_validator` objects, we can use inidividual `field_validator` utilities to do per-argument auto-casting. The effect of this is that we can then use positional constructor arguments without using the funcationality and dealing the full model `ArgsKwargs` objects.
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/476/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
=======================================
Coverage 96.03% 96.03%
=======================================
Files 226 226
Lines 40581 40587 +6
=======================================
+ Hits 38972 38978 +6
Misses 1609 1609
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -257,17 +257,10 @@ class InternalNode(Node, _InternalNode): | |||
|
|
|||
| _traversable = ['body'] | |||
|
|
|||
| @model_validator(mode='before') | |||
| @field_validator('body', mode='before') | |||
There was a problem hiding this comment.
Likely my ignorance of pydantic showing here: The pre_init method did also stuff to args and kwargs before. Is this implicitly handled now or not required anymore?
There was a problem hiding this comment.
Yes, but this one worked on the entire "Model" in pydantic speak, so it got a single ArgsKwargs argument that held all the constructor arguments as ArgsKwarg.args and ArgsKwargs.kwargs. This became an issue, as the ArgsKwargs object is immutable, but the ArgsKwarg.kwargs dict could be changed. This means, if any kw-args were given, I could change the dict, but I cannot replace None with () as ArgsKwargs.args = () is not allowed.
Long story short, this new validator is applied to each arg individual when pydantic creates the ArgsKwargs object, so all our troubles fade away and pydantic takes care of identifying unnamed args for free.
| arguments: Optional[Tuple[Expression, ...]] = () | ||
| kwarguments: Optional[Tuple[Tuple[str, Expression], ...]] = () |
There was a problem hiding this comment.
Is this a change in behaviour, i.e., arguments and kwarguments always a tuple now instead of None? (I'm not opposed, just trying to understand the impact)
There was a problem hiding this comment.
Internally, we would always force this into () in the pre-validator autocast mechanism. The reason I need to change it here is that the before-validator does not get invoked if the arg is omitted so the default is used directly. This should be agnostic to the previous behaviour. 🤞
loki/ir/tests/test_ir_nodes.py
Outdated
| @@ -148,6 +154,14 @@ def test_conditional(scope, n, a_i): | |||
| ) | |||
| assert not cond.body and cond.else_body == (assign, assign, assign) | |||
|
|
|||
| # Test auto-casting with unnamed constructor args | |||
| cond = ir.Conditional(condition) | |||
| assert cond.body == () and not cond.else_body | |||
There was a problem hiding this comment.
Pylint doesn't like cond.body == (), maybe
| assert cond.body == () and not cond.else_body | |
| assert isinstance(cond.body, tuple) and not (cond.body or cond.else_body) |
There was a problem hiding this comment.
Same here, cond.body is () should do to appease pylint...
loki/ir/tests/test_ir_nodes.py
Outdated
| @@ -235,6 +249,12 @@ def test_section(n, a_n, a_i): | |||
| sec = ir.Section((assign, (func,), assign, None)) | |||
| assert sec.body == (assign, func, assign) | |||
|
|
|||
| # Test auto-casting with unnamed constructor args | |||
| sec = ir.Section() | |||
| assert sec.body == () | |||
There was a problem hiding this comment.
| assert sec.body == () | |
| assert isinstance(sec.body, tuple) and not sec.body |
There was a problem hiding this comment.
Or even just sec.body is ()...
reuterbal
added
the
ready to merge
Instead of using
model_validatorobjects, we can use inidividualfield_validatorutilities to do per-argument auto-casting. The effect of this is that we can then use positional constructor arguments without using the funcationality and dealing the full modelArgsKwargsobjects.This should address issue #420, @wertysas to confirm.