-
Notifications
You must be signed in to change notification settings - Fork 13
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
base: main
Are you sure you want to change the base?
Conversation
5df5242
to
ad75311
Compare
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/453/index.html |
I skimmed it briefly and I really like the implementation! Would it make sense to have instead of |
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
That's not a bad idea, however, Other than that, what would the various options be? I can think of:
|
Naming things is always the hardest part ... |
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 theloki-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).