-
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
Generic enrichment process #189
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/189/index.html |
76f6d3e
to
e3f459a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
==========================================
+ Coverage 92.16% 92.21% +0.04%
==========================================
Files 93 93
Lines 16792 16835 +43
==========================================
+ Hits 15477 15524 +47
+ Misses 1315 1311 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e3f459a
to
50de1be
Compare
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.
Very neat and very useful. It also seems to "fix" some odd performance regression in large batch processing scenarios (ec-physics regression test). I left one small request to increase the log-level to capture this in the future, but otherwise this looks great and is GTG from my side. 👏 🙏
loki/bulk/scheduler.py
Outdated
for item in reversed(list(nx.topological_sort(self.item_graph))): | ||
item.source.make_complete(**build_args) | ||
build_args['definitions'] += item.source.definitions | ||
|
||
|
||
@Timer(logger=perf, text='[Loki::Scheduler] Enriched call tree in {:.2f}s') |
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.
During regression testing I noticed that this has previously become somewhat expensive, but this PR seems to remedy this. Could you please increase the log level here to info
, so that any eventual runtime increase gets flagged by default?
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.
Thanks, good point. Fixed.
e5cfa7a
to
be09a3f
Compare
Another lift of functionality from the Scheduler overhaul.
This one adds a more generic enrichment process. It is anchored on the following concepts:
USE
), it is sufficient to look at the import nodes and update the symbol table entries of the symbols listed there (or for all symbols from the remote module if no only list is given). This applies to both, modules and subroutines (we haven't done enrichment for the first before) and also gives us typedef and global variable enrichment.Subroutine
s (the only ones whereCallStatements
can appear), we additionally apply the previous enrichment via call statements toa. retain the
active
property andb. enrich procedure symbols that are made available without an import (i.e., via explicit interface or non-inlined interface block include)