The new Scheduler #213

reuterbal · 2024-01-18T13:23:05Z

Draft PR to trigger CI tests for now

github-actions · 2024-01-18T13:25:34Z

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

codecov · 2024-01-18T14:26:28Z

Codecov Report

Attention: Patch coverage is 94.35084% with 57 lines in your changes are missing coverage. Please review.

Project coverage is 92.78%. Comparing base (3fe6ce2) to head (1e03830).

Files	Patch %	Lines
loki/bulk/item.py	93.22%	32 Missing ⚠️
loki/bulk/scheduler.py	95.81%	12 Missing ⚠️
loki/bulk/configure.py	94.84%	5 Missing ⚠️
loki/program_unit.py	78.57%	3 Missing ⚠️
loki/sourcefile.py	86.66%	2 Missing ⚠️
loki/transform/build_system_transform.py	33.33%	2 Missing ⚠️
transformations/transformations/data_offload.py	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
+ Coverage   92.53%   92.78%   +0.25%     
==========================================
  Files          96       96              
  Lines       17580    17965     +385     
==========================================
+ Hits        16267    16669     +402     
+ Misses       1313     1296      -17

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`92.77% <94.34%> (+0.30%)`	⬆️
transformations	`91.94% <94.44%> (-0.05%)`	⬇️

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 · 2024-02-12T20:25:45Z

Just a little heads-up: I force-pushed after rebasing over latest main, to make this branch compatible with GlobalVarHoist and the switch to convert for F2C processing.
This branch includes also two temporary commits to point to different ecwam/cloudsc branches - these should be removed before (squash) merging the new Scheduler

reuterbal · 2024-02-22T12:26:52Z

This has been rebased again to integrate the changes from #222 and other PRs that were merged in the meantime. I also took the opportunity to squash the lengthy commit history and only retain the (to be removed) CLOUDSC and ecWAM branch redirection and the temporary skip of OMNI for CLOUDSC regression.

mlange05 · 2024-03-16T05:06:29Z

loki/bulk/scheduler.py

+        """
+        Create and return the :any:`SGraph` constructed from the :attr:`seeds` of the Scheduler.
+        """
+        return SGraph.from_seed(self.seeds, self.item_factory, self.config)


As discussed offline, this automatic re-generation of the SGraph is very expensive at the moment, even though it does seem to correctly use the caching mechanism provided by the ItemFactory. My suspicion is the use of Item.dependencies triggering a complete re-parse of each edge/node of the base graph via the REGEX frontend, as this always seems to trigger Item.concretize_dependencies.

In this current stage this effect makes prohibitively expensive (enrichment triggers about 6 rebuilds for me at around 40s each in ec-physics), which can be mitigated by not caching the sgraph. However, I wonder if there is a way to not "concretize" things that are already concrete, and if this would alleviate the problem somewhat?

I think you might be right!
The intention is to call make_complete to ensure the relevant parser classes have been parsed already - which should be a no-op after the initial discovery. For that reason, there is this bail-out here:

loki/loki/program_unit.py

Lines 290 to 291 in b101847

if self._parser_classes == parser_classes:

return

Or at least, that's what it is intended to be. We call make_complete only with the Item._depends_class, which ideally will be a subset of what has been parsed already. But that's not what we are checking for... If my half-asleep brain is not completely wrong, this line should instead read as:

if self._parser_classes & parser_classes:

Would you mind trialling this?

mlange05 · 2024-03-16T05:21:36Z

loki/bulk/item.py

        """
-        return self.config.get('expand', False)
+        if not (dependencies := self.dependencies):


So, I'm not 100% that I'm understanding this right, but to illustrate my point on expensive SGraph rebuilds, I believe this lines is triggered for determining every edge in the base graph, and will cause a regex-parse of every dependency item (via concretize_dependencies), regardless of whether this has been done before. I've not fully confirmed this, other than by observing that this call can take up to 2s for certain items.

If this is the case, I'm not super-sure how to fix this, but it feels like this should be fixable.

Ok for now, follow-up tracked via #254 .

WIP: Towards SGraph Expose defined IR nodes in Module and Subroutine Incremental parsing stubs via Item.definitions Initial sources for batch processing tests Link items to parser classes and name dependencies/definitions Expose global variables declared in a module Introduce an empty RegexParserClass First draft of on-demand regex logic and graph build Homogenize access to typedefs and imports in program units regex: parse kind in declarations On-demand graph building improved and more tests Add a rudimentary SGraph implementation Refactored item creation Enhance the SchedulerConfig with accessors Enhance Item creation with config and support for `disable` Expand disable testing Some documentation on new items Support for ignore and block in SGraph Fix typos and Linter warnings Resilience against items that cannot be found in non-strict discovery Python 3.8 compatibility Introduce file graph for SGraph Additional testing for nested typebound procedures WIP: Replace Scheduler callgraph by SGraph and SFilter Update linter test for module dependencies in scheduler Apply new enrichment procedure in scheduler Restore targets functionality Skip intrinsic modules Restore depths functionality Adapt to new Scheduler API Consistent use of bind_names for procedure bindings Support for inline call dependencies New, generic enrichment process On-the-fly creation of SGraph Pool Allocator compliance Item renaming post-rebase fixes wildcard disable in sgraph WIP: Support for Item renaming Crude support for cross-library ignores Get Dependency transformations to work Retire GlobalVariableItem WIP: Semantics for Item rename/create and dependency transformation fixes GlobalVarImportTransformation tests updated for Scheduler rewrite Fix successors for "edge" nodes Fix targets pruning in Sgraph Update CMake test with header dependency Lift item config into ItemConfig base class Pass ignore list instead of flag Split get_procedure_binding_item from get_procedure_item Use IR properties instead of bespoke Item properties/methods Overload creation of definitions for FileItem Remove redundant ProcedureBindingItem create step Make ProcedureItem and ProcedureBindingItem creating classmethods Convert create_from_ir to classmethod Move item creation to ItemFactory Move item_cache into ItemFactory Remove cached_property for Item.scope Remove redundant dependency injection mechanism Documentation for the new scheduler pylint compliance Remove some remnants of SubroutineItem Expand config value matching and apply correct tree pruning Fix typos Rename SubroutineItem to ProcedureItem in recently changed code Enable source sanitizer for incremental parse via from_source Add a test to verify that interface items depend on procedures Filter `items` kwarg to include only SGraph items in file_graph traversals Remove inactive items in DependencyTransformation Make GlobalVarHoistTrafo compatible with new scheduler F2C Trafo: Create a copy for C kernel Fix C kernel generation in F2C trafo Fix test_transpile_expression Add ignored items to dependency graph and skip during processing

mlange05

Ok, first of all this is an impressive piece of live-wire engineering, and the improvements from the SGraph, and the incremental parsing infrastructure are very powerful! Obviously, this type of major change is too large to do a detailed line-by-line review, but having played with it for a little while, I'm confident that any remaining issues can be addressed in follow-up PRs. So, I've left a couple of docstring pointers, but they are truly minor, so GTG from me!

And just to re-iterate, this is a major breaking change, so integration strategy and corresponding point-release management should happen as discuss offline (roll minor point release first, merge this, increment major point version number in quick follow-up release.

And again, many thanks @reuterbal for a truly impressive feature! 😁

mlange05 · 2024-03-21T08:27:57Z

docs/source/transform.rst

-functions that can be readily used in a step of the transformation pipeline:
+Most transformations, however, will only require modifying those parts of a file
+that are part of the call tree that is to be transformed to avoid unexpected
+side-effects. For that reason,


sentence not quite finished?

Indeed, but don't remember where I was going with this...

mlange05 · 2024-03-21T08:28:40Z

docs/source/transform.rst

-making sure that routines with the relevant :any:`CallStatement` are always
-processed before their target :class:`Subroutine`.
+:any:`Transformation` applies this transformation to all files, modules, or
+routines that are appear in the dependency graph. The exact traversal


that ~~are~~ appear

mlange05 · 2024-03-21T08:31:50Z

loki/bulk/item.py

        """
-        return self.config.get('expand', False)
+        if not (dependencies := self.dependencies):


Ok for now, follow-up tracked via #254 .

reuterbal · 2024-03-22T09:56:58Z

Documentation updated, will now rebase to remove pointers to temporary regression test branches, and then merge to main.

The default filtering for both, the cmake planner and the default source parse, now by default restricts itself to only the call-tree, but can be extended to include module-imports.

reuterbal force-pushed the nabr-the-new-scheduler branch 2 times, most recently from 36f2812 to 009f189 Compare January 18, 2024 14:01

reuterbal force-pushed the nabr-the-new-scheduler branch from 009f189 to 8c9b280 Compare January 18, 2024 14:55

reuterbal mentioned this pull request Jan 25, 2024

f2c transpile via convert #208

Merged

reuterbal force-pushed the nabr-the-new-scheduler branch from 0647d2e to cd7085d Compare January 31, 2024 19:31

reuterbal force-pushed the nabr-the-new-scheduler branch from 8e7d368 to e0601ca Compare February 12, 2024 20:23

reuterbal force-pushed the nabr-the-new-scheduler branch 2 times, most recently from 8c505db to cdf3441 Compare February 22, 2024 12:24

reuterbal marked this pull request as ready for review February 22, 2024 13:06

reuterbal requested review from mlange05, MichaelSt98 and awnawab February 22, 2024 13:07

mlange05 reviewed Mar 16, 2024

View reviewed changes

reuterbal force-pushed the nabr-the-new-scheduler branch from b101847 to 833fb3e Compare March 18, 2024 12:44

mlange05 mentioned this pull request Mar 20, 2024

Performance of (re-)generating large SGraphs #254

Open

mlange05 approved these changes Mar 21, 2024

View reviewed changes

This was referenced Mar 21, 2024

Compatibility with Loki v0.2 ecmwf-ifs/dwarf-p-cloudsc#74

Merged

Compatibility with Loki v0.2 ecmwf-ifs/ecwam#12

Merged

reuterbal added 5 commits March 22, 2024 11:06

Temporarily disable OMNI for CLOUDSC regression

c0ece10

config-file overwrite of frontend_args

408b620

Add a test for frontend overwrite via scheduler config

a162ade

Re-enable OMNI in CLOUDSC regression test

0422ed5

Fix typo in tests

8492696

reuterbal and others added 14 commits March 22, 2024 11:06

Docstring for create_frontend_args

8a293b3

Add ExternalItem to allow building dependency graph with missing nodes

ce8a4d5

Make ModuleWrapTransformation work with ExternalItem

a526ed3

Remove dependency on modules for procedure imports

4a49882

Source: Only log make_complete times for non-REGEX frontends

d00faf5

Scheduler: Avoid self-reference cycles in SGraph

750d84f

Scheduler: Filter the SGraph to calls-only when generating CMake plan

fe80329

Scheduler: Don't cache SGraph, but instead re-build when needed

1ababe5

Scheduler: Re-introduce enable_imports option via item_filter

9294da1

The default filtering for both, the cmake planner and the default source parse, now by default restricts itself to only the call-tree, but can be extended to include module-imports.

Scheduler: Drop redundant log-line in cmake planner

a20ca5c

Scheduler: Rebuild SGraph after discovery and parsing

f1ae277

Scheduler: Enable import-chasing in scheduler tests

d91af6c

DependencyTransformation: Ensure tuples when adding "block" entries

12a3e43

Additional Scheduler documentation

1e03830

reuterbal force-pushed the nabr-the-new-scheduler branch from 5dbd1fb to 1e03830 Compare March 22, 2024 10:07

reuterbal merged commit 338a042 into main Mar 22, 2024
8 of 12 checks passed

reuterbal deleted the nabr-the-new-scheduler branch March 22, 2024 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The new Scheduler #213

The new Scheduler #213

reuterbal commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

reuterbal commented Feb 12, 2024

reuterbal commented Feb 22, 2024

mlange05 Mar 16, 2024

reuterbal Mar 18, 2024 •

edited

Loading

mlange05 Mar 16, 2024

mlange05 Mar 21, 2024

mlange05 left a comment

mlange05 Mar 21, 2024

reuterbal Mar 22, 2024

mlange05 Mar 21, 2024

mlange05 Mar 21, 2024

reuterbal commented Mar 22, 2024

The new Scheduler #213

The new Scheduler #213

Conversation

reuterbal commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

reuterbal commented Feb 12, 2024

reuterbal commented Feb 22, 2024

mlange05 Mar 16, 2024

Choose a reason for hiding this comment

reuterbal Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

mlange05 Mar 16, 2024

Choose a reason for hiding this comment

mlange05 Mar 21, 2024

Choose a reason for hiding this comment

mlange05 left a comment

Choose a reason for hiding this comment

mlange05 Mar 21, 2024

Choose a reason for hiding this comment

reuterbal Mar 22, 2024

Choose a reason for hiding this comment

mlange05 Mar 21, 2024

Choose a reason for hiding this comment

mlange05 Mar 21, 2024

Choose a reason for hiding this comment

reuterbal commented Mar 22, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

reuterbal Mar 18, 2024 •

edited

Loading