Static Transformation properties (manifest) #154

mlange05 · 2023-09-18T07:33:58Z

This PR changes the way we configure the Scheduler behaviour for individual Transformation passes. Up until now, we rely on the user of a transformation to hand any special iteration properties (reverse graph traversal, program unit recursion, etc.) as run time argument to the scheduler, with no safeguards if the wrong behaviour is requested.

This PR now changes this, so that Transformation objects carry a set of specific flags as class properties (the manifest) that can be overridden in the class definition. So far, this change only moves scheduler traversal behaviour and behaviour specific to apply(...) to the manifest, but it can easily be extended to capture other static meta-information, etc.

In more detail:

Moved reverse_traversal, traverse_file_graph and item_filter properties to the static class manifest. @reuterbal Please note that these are the main "graph traversal" configuration options that should eventually be implemented via the planned SGraph mechanisms.
Move the "program unit recursion" properties to the manifest, with fine-grained configuration behaviour over which recursion is required. Please note that this includes a few mechanisms specific to the DependencyTransformation, which we can hopefully simplify once that transformation is streamlined and/or we have the SGraph (@reuterbal )
Fixes to various tests, and use of new configuration mechanism in DependencyTransformation and GlobalVarTransformation

github-actions · 2023-09-18T07:37:20Z

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/154/index.html

codecov · 2023-09-19T12:53:11Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (955a9da) 92.21% compared to head (906e5d4) 92.18%.

Files	Patch %	Lines
loki/transform/transformation.py	87.09%	4 Missing ⚠️
loki/transform/build_system_transform.py	66.66%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #154      +/-   ##
==========================================
- Coverage   92.21%   92.18%   -0.04%     
==========================================
  Files          93       93              
  Lines       16839    16881      +42     
==========================================
+ Hits        15528    15561      +33     
- Misses       1311     1320       +9

Flag	Coverage Δ
lint_rules	`96.22% <ø> (ø)`
loki	`92.14% <87.03%> (-0.05%)`	⬇️
transformations	`91.45% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mlange05 · 2023-11-17T13:31:05Z

Ok, after quite some fixing and tweaking (some workarounds needed to be shifted, rather than resolved), the test base is now green. I've also rebased over latest main. So, I think this is worth another look @reuterbal ?

reuterbal

Sorry for the latency, this is really great and a huge step forward towards a more formal procedure for planning and applying transformation passes. Overall no objections to any of the changes.

I left a few remarks where I think the naming could be improved, but none of that is a hard requirement. I also noticed that we don't currently cover some of the recursion control flows in the test base but this was presumably the case already before, so not necessarily required to act on this now.

My main concern would be whether we really need the "wrong" metadata provided in the transformation invocation during file graph traversals. See inline comment for details.

reuterbal · 2023-11-29T10:37:47Z

loki/transform/transformation.py

+    # Recursion behaviour when invoking transformations via ``trafo.apply()``
+    recurse_to_modules = False  # Recurse from Sourcefile to Module
+    recurse_to_subroutines = False  # Recurse from Sourcefile/Module to Subroutine
+    recurse_to_contained_procedures = False  # Recurse to subroutines in ``contains`` clause


I would propose to use this opportunity to call this recurse_to_internal_procedures or something like this. Module procedures are also in a contains clause inside the module, so this might be a little counter-intuitive otherwise.

reuterbal · 2023-11-29T10:38:35Z

loki/transform/transformation.py

+
+    # Recursion behaviour when invoking transformations via ``trafo.apply()``
+    recurse_to_modules = False  # Recurse from Sourcefile to Module
+    recurse_to_subroutines = False  # Recurse from Sourcefile/Module to Subroutine


Should we call this recurse_to_procedures? I know this is inconsistent with current API but would pre-empt another API change on this end with the planned renaming.

reuterbal · 2023-11-29T10:38:56Z

loki/transform/transformation.py

+    how the transformations are invoked for a given type of scheduler
+    :any:`Item`.
+
+    * ``reverse_traversal`` :


Sphinx supports an Attributes block, which I think we should use here.

reuterbal · 2023-11-29T10:42:10Z

loki/bulk/scheduler.py

+                    transformation.apply(
+                        items[0].source, role=_item.role, mode=_item.mode,
+                        item=_item, items=items, targets=_item.targets,
+                    )


Do we need to return to passing items[0]'s properties to the transformations here? I removed this not too long ago because it seemed error-prone to have a single item's properties used for the entire file, and it was not actually required by any transformation.

Ah, yes you are right. This was actually a workaround for a nasty bug. I found it now and reverted this (PEBKAC!).

reuterbal · 2023-11-29T10:54:58Z

loki/transform/transformation.py

+    # Forces scheduler traversal in reverse order from the leaf nodes upwards
+    reverse_traversal = False
+
+    # Traverse a graph of Sourcefile options corresponding to scheduler items
+    traverse_file_graph = False


I wonder if we may encounter additional traversal requirements in the future? In which case another variant of specifying these could be to introduce an enum.Flag which has orthogonal traversal options REVERSE_TRAVERSAL, FILE_GRAPH_TRAVERSAL etc. and then allows to combine them in a single traversal attribute on Transformation that is queried by the scheduler. Introducing an additional traversal "mode" would then only require adding the relevant flag.

Yeah, in principle I agree, but I think we might want to wait with this until the SGraph is done, as it will introduce more traversal mechanisms. For the sake of it, I gave this a quick try, but it introduces yet more circular dependencies, so I think we might want to postpone for a dedicated PR?

Agreed, leave as is and we can pick this up when it actually becomes relevant.

Instead of requiring the the user to correctly flag reverse traversal with the scheduler, we now set a class attribute flag that the scheduler check for reversed traversal.

Since we potentially have a one-to-many mapping here, we simply use the first `item` to provide the meta-data. This is a bit hacky, but needs full SGraph development to be fixed "properly".

This now resembles the more formal nomenclature of "procedures" and "internal procedures", which we are trying to adopt throughout.

reuterbal

Many thanks, happy with the current state!

mlange05 requested a review from reuterbal September 18, 2023 07:33

mlange05 force-pushed the naml-transformation-manifest branch 7 times, most recently from 3b7f84e to 9a9478e Compare September 19, 2023 12:20

mlange05 force-pushed the naml-transformation-manifest branch 5 times, most recently from fab5ef8 to 7e6a44d Compare November 17, 2023 10:19

reuterbal requested changes Nov 29, 2023

View reviewed changes

mlange05 added 7 commits November 30, 2023 09:33

Transformation: Make reverse_traversal a class attribute

7785649

Instead of requiring the the user to correctly flag reverse traversal with the scheduler, we now set a class attribute flag that the scheduler check for reversed traversal.

Transformation: Make use_file_graph a class attribute

dcce803

Transformation: Make item_filter a class attribute

d1a8ffd

Transformation: Make program unit recursion configurable

5531d81

Transformation: Document new manifest properties in docstring

0e4c8c5

Scheduler: Provide full meta-data for file-graph traversals

8c578a4

Since we potentially have a one-to-many mapping here, we simply use the first `item` to provide the meta-data. This is a bit hacky, but needs full SGraph development to be fixed "properly".

Transforamtion: Re-instate per-item recursion for complex traversals

99043a0

mlange05 force-pushed the naml-transformation-manifest branch from 7e6a44d to 99043a0 Compare November 30, 2023 08:33

mlange05 added 2 commits November 30, 2023 10:00

Transformation: Renaming recursion attributes and improve docstring

0faa31d

This now resembles the more formal nomenclature of "procedures" and "internal procedures", which we are trying to adopt throughout.

Transformation: Fixing metadata for recursion with mutliple items

906e5d4

mlange05 force-pushed the naml-transformation-manifest branch from 8a180bc to 906e5d4 Compare December 1, 2023 12:53

reuterbal approved these changes Dec 4, 2023

View reviewed changes

reuterbal added the ready to merge This PR has been approved and is ready to be merged label Dec 4, 2023

reuterbal merged commit 3a06456 into main Dec 5, 2023
11 checks passed

reuterbal deleted the naml-transformation-manifest branch December 5, 2023 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static Transformation properties (manifest) #154

Static Transformation properties (manifest) #154

mlange05 commented Sep 18, 2023

github-actions bot commented Sep 18, 2023

codecov bot commented Sep 19, 2023 •

edited

Loading

mlange05 commented Nov 17, 2023

reuterbal left a comment

reuterbal Nov 29, 2023

reuterbal Nov 29, 2023

reuterbal Nov 29, 2023

reuterbal Nov 29, 2023

mlange05 Dec 1, 2023

reuterbal Nov 29, 2023

mlange05 Dec 1, 2023

reuterbal Dec 4, 2023

reuterbal left a comment

Static Transformation properties (manifest) #154

Static Transformation properties (manifest) #154

Conversation

mlange05 commented Sep 18, 2023

github-actions bot commented Sep 18, 2023

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

mlange05 commented Nov 17, 2023

reuterbal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reuterbal left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2023 •

edited

Loading