Pipeline-plan duplicate/remove transformation changing dependencies

[MichaelSt98]

Proof-of-concept transformations DuplicateKernel and RemoveKernel with both changing the dependencies for the scheduler.

This builds on top of #453

DuplicateKernel takes a kernel and creates a duplicated kernel with corresponding duplicated items and adds a call to this duplicated kernel.

RemoveKernel removes a kernel from the scheduler and also removes the call.

[github-actions]

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

[codecov]

Codecov Report

Attention: Patch coverage is 93.68723% with 44 lines in your changes missing coverage. Please review.

Project coverage is 93.38%. Comparing base (b0a2344) to head (534e575).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
loki/batch/item_factory.py	86.05%	35 Missing ⚠️
loki/transformations/dependency.py	92.53%	5 Missing ⚠️
loki/transformations/tests/test_dependency.py	99.13%	2 Missing ⚠️
loki/frontend/source.py	87.50%	1 Missing ⚠️
loki/sourcefile.py	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #462      +/-   ##
==========================================
+ Coverage   93.35%   93.38%   +0.03%     
==========================================
  Files         223      226       +3     
  Lines       41428    41906     +478     
==========================================
+ Hits        38676    39136     +460     
- Misses       2752     2770      +18     

Flag	Coverage Δ
lint_rules	96.39% <ø> (ø)
loki	93.34% <93.68%> (+0.03%)	⬆️

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.

[MichaelSt98]

@mlange05 and @reuterbal I would wait for some short feedback before I add more tests to improve code coverage

[reuterbal]

Many thanks for this, this is a very useful demonstration of this functionality!

Conceptually, there's a few gotchas and things that I would prefer to do differently, and I'll try to explain here:

1. Item creation should always go through the item_factory, and the introduced clone method shortcuts this. Instead, we should have a get_or_create_from_item method or similar on the ItemFactory, to make sure any new items automatically land in the cache.

2. You inject the dependency changes incurred from plan_data in the dependencies property. However, that property returns (and I would like to keep it that way) IR nodes that constitute the dependency, e.g., CallStatement, Import, etc. So, we have two options: (a) inject (and remove) relevant IR nodes during planning to fake these dependencies or (b) inject the plan dependencies at a point where we are already working with items. I think (b) is a little easier and I have explored doing this via create_dependency_items instead, which seems to work (more details below).

3. The need for a recursive clone can be inferred from not specifying an IR to the clone method. For contained subroutines in ProgramUnit we are already doing this, and I would prefer to do the same for source files instead of introducing a new rebuild-method on scoped nodes.

4. We need to separate plan and trafo in the tests - both need to be able to apply the relevant sgraph topology changes independently, because we run either plan or trafo mode in practice. Getting both to work, i.e., trafo after plan, might be rather tricky but I also don't see a need for that at the moment.

Admittedly, half the observations above I only figured out when starting to play with this directly and trying to apply the relevant changes. So, I've decided not to bother you with too many inline comments that I kept editing and changing and I will instead file a PR on top of yours against your PR branch. I hope this doesn't come across as dismissive, but I've underestimated how fiddly this gets...

[reuterbal]

Ah, I forgot to delete the inline comments. Disregard them or at least don't action on them, yet :)

[MichaelSt98]

Many thanks for this, this is a very useful demonstration of this functionality!

Conceptually, there's a few gotchas and things that I would prefer to do differently, and I'll try to explain here:

1. Item creation should always go through the item_factory, and the introduced clone method shortcuts this. Instead, we should have a get_or_create_from_item method or similar on the ItemFactory, to make sure any new items automatically land in the cache.
2. You inject the dependency changes incurred from plan_data in the dependencies property. However, that property returns (and I would like to keep it that way) IR nodes that constitute the dependency, e.g., CallStatement, Import, etc. So, we have two options: (a) inject (and remove) relevant IR nodes during planning to fake these dependencies or (b) inject the plan dependencies at a point where we are already working with items. I think (b) is a little easier and I have explored doing this via create_dependency_items instead, which seems to work (more details below).
3. The need for a recursive clone can be inferred from not specifying an IR to the clone method. For contained subroutines in ProgramUnit we are already doing this, and I would prefer to do the same for source files instead of introducing a new rebuild-method on scoped nodes.
4. We need to separate plan and trafo in the tests - both need to be able to apply the relevant sgraph topology changes independently, because we run either plan or trafo mode in practice. Getting both to work, i.e., trafo after plan, might be rather tricky but I also don't see a need for that at the moment.

Admittedly, half the observations above I only figured out when starting to play with this directly and trying to apply the relevant changes. So, I've decided not to bother you with too many inline comments that I kept editing and changing and I will instead file a PR on top of yours against your PR branch. I hope this doesn't come across as dismissive, but I've underestimated how fiddly this gets...

That is perfectly fine for me and I wasn't expecting this PR to go in as it is but to serve as a first step/proof of concept.

[MichaelSt98]

Multi-level call tree duplication/removal for a follow-up PR.

[reuterbal]

Thanks for mopping up the outstanding housekeeping tasks, @MichaelSt98!

I think this is ready for integration, more testing and expansion to multi-level graph changes etc. should happen in follow-up PRs.

[mlange05]

Agreed! Looks great now and next steps go into follow-ons. Many thanks to both! GTG