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

Generic enrichment process #189

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Generic enrichment process #189

merged 5 commits into from
Nov 29, 2023

Conversation

reuterbal
Copy link
Collaborator

Another lift of functionality from the Scheduler overhaul.

This one adds a more generic enrichment process. It is anchored on the following concepts:

  1. For anything that is imported via a module import (i.e. 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.
  2. For anything that inherits its type from a parent (e.g., import in the parent module, call in the module procedure), we can compare symbol table entries with the parent's symbol table
  3. For Subroutines (the only ones where CallStatements can appear), we additionally apply the previous enrichment via call statements to
    a. retain the active property and
    b. enrich procedure symbols that are made available without an import (i.e., via explicit interface or non-inlined interface block include)

Copy link

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

@reuterbal reuterbal force-pushed the nabr-generic-enrichment branch 2 times, most recently from 76f6d3e to e3f459a Compare November 16, 2023 09:09
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a851213) 92.16% compared to head (be09a3f) 92.21%.
Report is 4 commits behind head on main.

Files Patch % Lines
loki/program_unit.py 95.00% 2 Missing ⚠️
loki/bulk/scheduler.py 93.75% 1 Missing ⚠️
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     
Flag Coverage Δ
lint_rules 96.22% <ø> (ø)
loki 92.18% <96.34%> (+0.05%) ⬆️
transformations 91.43% <ø> (ø)

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 reuterbal changed the title Generich enrichment process Generic enrichment process Nov 16, 2023
@reuterbal reuterbal marked this pull request as ready for review November 16, 2023 12:23
Copy link
Collaborator

@mlange05 mlange05 left a 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. 👏 🙏 :shipit:

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')
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good point. Fixed.

@reuterbal reuterbal added the ready for merge This PR has been approved and is ready to be merged label Nov 28, 2023
@reuterbal reuterbal merged commit 04b7c1c into main Nov 29, 2023
12 checks passed
@reuterbal reuterbal deleted the nabr-generic-enrichment branch November 29, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants