Disable default recursion for Transformation.apply #136

reuterbal · 2023-09-01T08:52:05Z

This is a proposal to fix #131.

By default, the Transformation class recurses to all contained nodes, i.e., calling apply on a Sourcefile recurses into all Module and Subroutine nodes in that file, and so on. Arguably, this was a questionable design choice, however, the Scheduler.process method relies on this behaviour to apply transformations. There, we invoke the transformation always on the source file that contains the IR node corresponding to an Item. (And then do some silly filtering afterwards, which possibly even prevents us from applying transformations to contained member subroutines...? 🤔)

This PR switches off the recursion by default but allows to retain current behaviour via the recurse_to_contained_nodes option.

I'll file a separate PR on top of the current one that changes also the Scheduler behaviour by directly applying transformations only to the IR node corresponding to a scheduler graph item. This requires some more careful regression testing for ecphys, and therefore this might only be sensible to do with the new Scheduler (hence the separate PR).

Important to note is that the change does have API implications when not using the Scheduler, because it requires to be more careful to which IR node a Transformation is applied - by either explicitly applying it to the target file, module, or subroutine directly, or explicitly enabling the recursion.

github-actions · 2023-09-01T08:55:25Z

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

codecov · 2023-09-01T09:33:13Z

Codecov Report

Merging #136 (87bd488) into main (75fc36f) will decrease coverage by 0.04%.
The diff coverage is 87.23%.

@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
- Coverage   92.12%   92.08%   -0.04%     
==========================================
  Files          89       89              
  Lines       16508    16533      +25     
==========================================
+ Hits        15208    15225      +17     
- Misses       1300     1308       +8

Flag	Coverage Δ
lint_rules	`96.22% <ø> (ø)`
loki	`92.03% <87.23%> (-0.02%)`	⬇️
transformations	`91.48% <ø> (-0.22%)`	⬇️

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

Files Changed	Coverage Δ
loki/transform/transformation.py	`90.47% <ø> (-1.31%)`	⬇️
loki/transform/fortran_c_transform.py	`87.72% <28.57%> (-1.54%)`	⬇️
loki/bulk/scheduler.py	`81.17% <87.50%> (+0.08%)`	⬆️
loki/transform/build_system_transform.py	`98.18% <100.00%> (+0.18%)`	⬆️
loki/transform/dependency_transform.py	`96.93% <100.00%> (+0.33%)`	⬆️

... and 5 files with indirect coverage changes

… item if called via recursion

reuterbal · 2023-09-08T19:17:56Z

Merged #137 and rebased over main

mlange05

Ok, first of all many thanks for sticking with it through this change. This one was quite tricky, because the underlying assumptions of the DependencyTransformation are basically broken and ill designed (for context, this transformation also predates most of the scheduler "smarts").

Nevertheless, full removing recursion and letting transformations implement the complex file=>module=>routine=>member recursion manually is a good move, for now, until we have more flexibility to configure transformations generically (the "manifest" idea).

But, there is a lot(!) of great housekeeping, testing and general quality of life stuff in here, so GT to merge this, and I'll attempt any simplification and clean-up on top of this.

mlange05 · 2023-09-09T06:21:11Z

loki/transform/dependency_transform.py

+        is_member = isinstance(routine.parent, Subroutine)
+
+        # Pick out correct item for current subroutine if processed via recursion
+        if 'items' in kwargs:


I do not understand this logic here: Why are we recovering item from items if item is given by the scheduler?

That's gone after yesterday's push, the file entry point is the place now where we do the item-picking and dispatch to program unit-specific entry points.

Yes, and after diving through this a little more, I think I understand how/why this was done.

mlange05 · 2023-09-12T07:25:03Z

scripts/loki_transform.py

@@ -29,9 +28,6 @@
 )

 # pylint: disable=wrong-import-order
-from transformations.argument_shape import (
-    ArgumentArrayShapeAnalysis, ExplicitArgumentArrayShapeTransformation


These should not have been removed. This is an error in #142 and is fixed in #149. Good catch, and thanks for "flagging" this though. 😉

Apologies for blindly trusting the linter instead of using my brain...

mlange05 · 2023-09-12T07:25:57Z

loki/bulk/scheduler.py

@@ -638,9 +650,16 @@ def process(self, transformation, reverse=False, item_filter=SubroutineItem, use
                    # Use entry from item_map to ensure the original item is used in transformation
                    _item = self.item_map[item.name]

+                    # Pick out the IR node to which to apply the transformation
+                    # TODO: should this become an Item property?


[no action] Yes, it should, but it requires the SGraph development to make this behaviour neat. So good for now.

mlange05 · 2023-09-12T07:27:38Z

loki/transform/dependency_transform.py

+        if items:
+            # Recursion into all subroutine items in the current file
+            for item in items:
+                self.transform_subroutine(item.routine, item=item, role=item.role, targets=item.targets, **kwargs)


[no action] This interwoven subtlety highlights that the DependencyTransform is actually deeply flawed and needs to be re-designed. As already discussed offline, like as a combination of several individual transformations, so thanks for keeping this functional! 🙏

mlange05 · 2023-09-12T07:31:01Z

tests/test_transform_dependency.py

+        driver = Sourcefile.from_source(driver_fcode, frontend=frontend)
+
+        # Because the renaming is intended to be applied to the routines as well as the enclosing module,
+        # we need to invoke the transformation on the full source file and activate recursion to contained nodes


With the removal of the recurse_to_contained_nodes option, this comment seems outdated. It's also repeated a few times throughout the test file.

Yes! I removed the branching in the test related to this but overlooked the comments. Will fix

…t renaming

reuterbal linked an issue Sep 1, 2023 that may be closed by this pull request

Calling a Transformation on a subroutine automatically applies itself recursively on contained subroutines. #131

Closed

reuterbal requested a review from mlange05 September 1, 2023 08:52

reuterbal force-pushed the 131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines branch from 18b61a4 to 47b86a8 Compare September 1, 2023 09:04

reuterbal force-pushed the 131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines branch from 47b86a8 to 562c63d Compare September 1, 2023 12:39

reuterbal mentioned this pull request Sep 1, 2023

Scheduler.process: apply directly to program unit corresponding to Item #137

Merged

reuterbal force-pushed the 131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines branch from 562c63d to 9fd7318 Compare September 6, 2023 12:26

reuterbal added 7 commits September 8, 2023 20:13

Disable default recursion for Transformation.apply

45fa710

Apply trafo in Scheduler.process directly to IR node

c82a1c2

DependencyTrafo: pick out roles, targets from item and select correct…

9273dac

… item if called via recursion

Add item filter to file graph scheduler traversal

5f4de4e

Move DependencyTransformation tests to new file

b68489f

DependencyTrafo: Add scheduler-based and contained member testing

0b488ba

DependencyTrafo: Do not process contained members

2889c4a

reuterbal force-pushed the 131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines branch from 35f6390 to 2889c4a Compare September 8, 2023 19:17

reuterbal added 3 commits September 11, 2023 15:09

Remove recurse_to_contained_nodes option

da37c45

Make F2C transpilation compatible with no recursion

2467095

Remove another property extraction

0ce23a0

mlange05 approved these changes Sep 12, 2023

View reviewed changes

DependencyTransform: Test header mod import and fix inline call impor…

87bd488

…t renaming

reuterbal force-pushed the 131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines branch from fdf59cd to 87bd488 Compare September 12, 2023 14:08

reuterbal merged commit 258eae5 into main Sep 13, 2023
12 checks passed

reuterbal deleted the 131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines branch September 13, 2023 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable default recursion for Transformation.apply #136

Disable default recursion for Transformation.apply #136

reuterbal commented Sep 1, 2023

github-actions bot commented Sep 1, 2023

codecov bot commented Sep 1, 2023 •

edited

Loading

reuterbal commented Sep 8, 2023

mlange05 left a comment

mlange05 Sep 9, 2023

reuterbal Sep 12, 2023

mlange05 Sep 12, 2023

mlange05 Sep 12, 2023

reuterbal Sep 12, 2023

mlange05 Sep 12, 2023

mlange05 Sep 12, 2023

mlange05 Sep 12, 2023

reuterbal Sep 12, 2023

Disable default recursion for Transformation.apply #136

Disable default recursion for Transformation.apply #136

Conversation

reuterbal commented Sep 1, 2023

github-actions bot commented Sep 1, 2023

codecov bot commented Sep 1, 2023 • edited Loading

Codecov Report

reuterbal commented Sep 8, 2023

mlange05 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

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2023 •

edited

Loading