-
Notifications
You must be signed in to change notification settings - Fork 12
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
RemoveCodeTransformation: make call_names and intrinsic_names configurable #289
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/289/index.html |
@@ -207,7 +207,8 @@ def convert( | |||
if not remove_code_trafo: | |||
remove_code_trafo = RemoveCodeTransformation( | |||
remove_marked_regions=True, remove_dead_code=False, | |||
call_names=('ABOR1', 'DR_HOOK'), intrinsic_names=('WRITE(NULOUT',) | |||
call_names=scheduler.config.default.get('call_names', None) or ('ABOR1', 'DR_HOOK'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awnawab I think you can just configure the transformation itself with all of it's constructor arguments in the config file as something like (untested):
[transformations.RemoveCodeTransformation]
module = "loki.transform"
[transformations.RemoveCodeTransformation.options]
remove_marked_regions = true
remove_dead_code = false
call_names =['dr_hook', 'abor1']
intrinsic_names = ['write(nulout']
This way, no custom assumptions or behaviour are added to the convert
step, which is current direction of travel (removing all the custom logic). Ultimately the plan was to remove all of this and configure the entire pipeline in config, but I've no full-scale example of this yet. Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - transformation-specific configuration should not be added to defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah, I had completely missed the addition of this functionality! Thanks for pointing this out, of course the above suggestion works and I'll close this PR 👍
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
=======================================
Coverage 92.89% 92.89%
=======================================
Files 102 102
Lines 18334 18334
=======================================
Hits 17032 17032
Misses 1302 1302
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This PR makes the
call_names
andintrinsic_names
constructor args in theRemoveCodeTransformation
configurable.