Skip to content
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

CMake Plan: Pipeline dry-run and transformation-based plan creation #453

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

reuterbal
Copy link
Collaborator

This is an alternative implementation suggestion to #444

The objective here would be to retain current functionality and implement support for transformations that change scheduler graph topology in subsequent PRs.

Please have a look and point out anything that looks odd to you. In particular, please consider the interface - e.g., is a plan argument acceptable and the naming ok?

With this, the loki-transform.py plan command is redirected to the loki-transform.py convert command, clearing the way for subsequent removal of the custom transformation entry point and further homogenisation in the CLI and CMake layer.

I will also add another commit to add some more documentation (and I expect the linter pass to fail due to the local import).

Copy link

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

@MichaelSt98
Copy link
Collaborator

I skimmed it briefly and I really like the implementation!

Would it make sense to have instead of process(self, transformation, plan=False) something like process(self, transformation, mode=<batch|normal|plan|...>)?

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 91.45299% with 10 lines in your changes missing coverage. Please review.

Project coverage is 93.28%. Comparing base (ba7e230) to head (ad75311).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
loki/batch/transformation.py 82.92% 7 Missing ⚠️
loki/transformations/build_system/file_write.py 88.23% 2 Missing ⚠️
loki/transformations/build_system/plan.py 97.22% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #453      +/-   ##
==========================================
- Coverage   93.28%   93.28%   -0.01%     
==========================================
  Files         220      221       +1     
  Lines       41202    41230      +28     
==========================================
+ Hits        38436    38461      +25     
- Misses       2766     2769       +3     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 93.24% <91.45%> (-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.

@reuterbal
Copy link
Collaborator Author

Would it make sense to have instead of process(self, transformation, plan=False) something like process(self, transformation, mode=<batch|normal|plan|...>)?

That's not a bad idea, however, mode as a name is also used to specify the "transformation mode", i.e., scc, scc-stack etc. Not sure if we would want to avoid that conflict?

Other than that, what would the various options be? I can think of:

  • plan: The dry-run mode from this PR
  • atomic (not sure that's a good name): The current mode of applying each transformation to every item before applying the next transformation
  • batch: Could be a future mode that applies to an item all transformations that share the same traversal mode before going to the next item, thus "batching" the processing. Out of scope for this PR, just to highlight a potential future mode.
  • default: Should then alias to whatever we think is the best default, e.g. atomic right now?

@MichaelSt98
Copy link
Collaborator

Would it make sense to have instead of process(self, transformation, plan=False) something like process(self, transformation, mode=<batch|normal|plan|...>)?

That's not a bad idea, however, mode as a name is also used to specify the "transformation mode", i.e., scc, scc-stack etc. Not sure if we would want to avoid that conflict?

Other than that, what would the various options be? I can think of:

  • plan: The dry-run mode from this PR
  • atomic (not sure that's a good name): The current mode of applying each transformation to every item before applying the next transformation
  • batch: Could be a future mode that applies to an item all transformations that share the same traversal mode before going to the next item, thus "batching" the processing. Out of scope for this PR, just to highlight a potential future mode.
  • default: Should then alias to whatever we think is the best default, e.g. atomic right now?

Naming things is always the hardest part ...
What about process_mode?
Yes, those options are what I was thinking about and I think atomic is a good name, however, we could also go with serial?
Moreover, we could think about calling transform_file/transform_routine or plan_file/plan_routine in dependence of mode/process_mode like calling <process_mode>_file/<process_mode>_routine, but this would probably be over-engineering right now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants