-
Notifications
You must be signed in to change notification settings - Fork 12
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.
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
IR: Automatic sanitisation of tuples in IR constructors #350
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/350/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #350 +/- ##
==========================================
+ Coverage 95.33% 95.35% +0.01%
==========================================
Files 171 171
Lines 36543 36801 +258
==========================================
+ Hits 34837 35090 +253
- Misses 1706 1711 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
b31ee40
to
a013e13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic, many thanks, and we finally gain the flexibility of being more forgiving in the tuple nesting in the constructor while ensuring correct IR nodes.
One question about a potential change in default behaviour for Conditional and whether this is intentional, otherwise good to go!
@@ -560,7 +570,7 @@ class _ConditionalBase(): | |||
|
|||
condition: Expression | |||
body: Tuple[Node, ...] | |||
else_body: Optional[Tuple[Node, ...]] = None | |||
else_body: Optional[Tuple[Node, ...]] = () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a change in behaviour for the default value of else_body
?
I think so far we used else_body=None
as indicator that there isn't an else branch in the conditional - as opposed to an empty else branch (which has the same result but different on a string level).
As in, will ir.Conditional(condition=<...>, body=<...>)
now be represented as the following?
IF (<...>) THEN
<...>
ELSE
ENDIF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we're doing a plain old if o.else_body:
which evaluates to the same thing:
https://github.com/ecmwf-ifs/loki/blob/main/loki/backend/fgen.py#L633
@reuterbal I just noted that we now have a hard dependency on |
Ok, fix pushed. Please note that existing local checkouts might fail after this - to remedy, a simple |
Just`` testing for now....Note, this is based on #349, as it requires pydantic version >=2.0.
Following the Pydantic upadte, this PR now smoothes over the type-checking of the base IR nodes, in particular
InternalNode
, and adds "smart" conversion of.body
tuples to the desired internal format. In particular, this now means that erroneous tuples (eg. nested, or containingNone
instances) are now automatically fixed, before being passed to the pydantic validators. This in turn will allow a more liberal use ofTransformers
for in-traversal replacements (the real point of this change!).In more detail:
_sanitise_tuple
utility method toloki.ir.nodes
.body
asTuple[Union[Node, Scope], ...]
to allow nestedSubroutine
objects (eg. internal procedures)Loop
andConditional
, and added test forSection
, which tests only the inherited behaviour fromInternalNode