Make builddir a runtime argument of `FileWriteTransformation` #425

awnawab · 2024-11-01T07:45:47Z

A small PR that turns the builddir into a runtime argument of the FileWriteTransformation. This allows the builddir to be passed in via command line arguments even if the FileWriteTransformation is being overridden via a scheduler config.

codecov · 2024-11-01T07:46:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.07%. Comparing base (899f5ac) to head (74d9660).
Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   93.04%   93.07%   +0.02%     
==========================================
  Files         198      198              
  Lines       39320    39422     +102     
==========================================
+ Hits        36587    36691     +104     
+ Misses       2733     2731       -2

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`93.02% <100.00%> (+0.02%)`	⬆️

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.

github-actions · 2024-11-01T07:48:56Z

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

mlange05

All in all this looks good. I've left one conceptual question for @reuterbal , but once answered this is GTG for me.

mlange05 · 2024-11-01T11:55:08Z

loki/batch/scheduler.py

@@ -495,7 +498,7 @@ def _get_definition_items(_item, sgraph_items):
                    _item.scope_ir, role=_item.role, mode=_item.mode,
                    item=_item, targets=_item.targets, items=_get_definition_items(_item, sgraph_items),
                    successors=graph.successors(_item, item_filter=item_filter),
-                    depths=graph.depths
+                    depths=graph.depths, build_args=self.build_args


[no action] I would argue that we could also attach the build_args to the Item, so that we continue to use the Item as the universal meta-data object that gets handed from Scheduler to Transformation? Admittedly, this can be said for almost any of the arguments here - so this might also be part of a larger "scheduler-item" refactoring and need not go into this PR.

Any strong opinions on this, @reuterbal ?

Yes, good idea.

The only thing where we need to be a little careful is that the definitions part is updated dynamically when calling _parse_items. When re-parsing due to topology changes that introduce a new dependency that was previously not part of the graph, these definitions would then be missing for existing items. Hence, this is probably out of scope for this PR.

For the other arguments it depends. role and targets should be used from the Item and have only been kept for backwards compatibility. We should indeed remove them here. The other two are properties at a graph-level, not an item level, so there's an argument to keep them separate.

Ok, agreed. I'll look into this in a separate PR. 👍

reuterbal

Thanks, looks great. A few nit-picks on nomenclature but otherwise good to go!

reuterbal · 2024-11-04T12:20:00Z

loki/batch/scheduler.py

    """

    # TODO: Should be user-definable!
    source_suffixes = ['.f90', '.F90', '.f', '.F']

    def __init__(self, paths, config=None, seed_routines=None, preprocess=False,
                 includes=None, defines=None, definitions=None, xmods=None,
-                 omni_includes=None, full_parse=True, frontend=FP):
+                 omni_includes=None, full_parse=True, frontend=FP, builddir=None):


I would suggest we use a more generic term than builddir here, for example output_path. There is no reason that this needs to be an actual build directory, and for Loki processing that is not integrated with a build the name wouldn't make much sense.
It's just that in our use case we happen to specify the build directory as the output path

How about output_dir? Since the full path of the file is constructed combining this and sourcepath, I think we should keep dir in the name.

Yes, good point! The paths argument suffer from the same misnomer but that's a separate issue

reuterbal · 2024-11-04T12:26:52Z

loki/batch/scheduler.py

@@ -495,7 +498,7 @@ def _get_definition_items(_item, sgraph_items):
                    _item.scope_ir, role=_item.role, mode=_item.mode,
                    item=_item, targets=_item.targets, items=_get_definition_items(_item, sgraph_items),
                    successors=graph.successors(_item, item_filter=item_filter),
-                    depths=graph.depths
+                    depths=graph.depths, build_args=self.build_args


Yes, good idea.

The only thing where we need to be a little careful is that the definitions part is updated dynamically when calling _parse_items. When re-parsing due to topology changes that introduce a new dependency that was previously not part of the graph, these definitions would then be missing for existing items. Hence, this is probably out of scope for this PR.

For the other arguments it depends. role and targets should be used from the Item and have only been kept for backwards compatibility. We should indeed remove them here. The other two are properties at a graph-level, not an item level, so there's an argument to keep them separate.

reuterbal · 2024-11-04T12:29:28Z

loki/transformations/build_system/file_write.py

@@ -72,6 +69,6 @@ def transform_file(self, sourcefile, **kwargs):
        path = Path(item.path)
        suffix = self.suffix if self.suffix else path.suffix
        sourcepath = Path(item.path).with_suffix(f'.{self.mode}{suffix}')
-        if self.builddir is not None:
-            sourcepath = self.builddir/sourcepath.name
+        if (builddir := kwargs.get('build_args', {}).get('builddir', None)) is not None:


I think we're be able to afford two lines to make this a little more readable :)

Suggested change

if (builddir := kwargs.get('build_args', {}).get('builddir', None)) is not None:

build_args = kwargs.get('build_args')

if build_args and (builddir := build_args.get('builddir')):

reuterbal · 2024-11-04T16:13:48Z

loki/batch/tests/test_transformation.py

@@ -428,8 +428,8 @@ def test_transformation_file_write(tmp_path):
    ricks_path = tmp_path/'rick.roll.java'
    if ricks_path.exists():
        ricks_path.unlink()
-    FileWriteTransformation(mode='roll', suffix='.java').apply(source=source, item=item,
-                                                               build_args={'output_dir': tmp_path})
+    FileWriteTransformation(suffix='.java').apply(source=source, item=item,


[no action!] The suffix should probably become an item property as well in the future, particularly when thinking of transpilation use cases?

awnawab added 2 commits October 31, 2024 16:15

Pass builddir to the Scheduler constructor

09e98bf

FileWrite: make builddir a runtime argument

9b6e88d

awnawab requested review from mlange05 and reuterbal and removed request for mlange05 November 1, 2024 07:45

mlange05 reviewed Nov 1, 2024

View reviewed changes

reuterbal requested changes Nov 4, 2024

View reviewed changes

FileWrite: rename builddir to output_dir

f6b3c18

reuterbal approved these changes Nov 4, 2024

View reviewed changes

reuterbal added the ready to merge This PR has been approved and is ready to be merged label Nov 4, 2024

reuterbal reviewed Nov 4, 2024

View reviewed changes

FileWrite: make mode a runtime arg too

74d9660

awnawab force-pushed the naan-file-write branch from 067ec82 to 74d9660 Compare November 4, 2024 16:33

reuterbal added ready to merge This PR has been approved and is ready to be merged and removed ready to merge This PR has been approved and is ready to be merged labels Nov 4, 2024

reuterbal merged commit b833fef into main Nov 5, 2024
13 checks passed

reuterbal deleted the naan-file-write branch November 5, 2024 06:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make builddir a runtime argument of `FileWriteTransformation` #425

Make builddir a runtime argument of `FileWriteTransformation` #425

awnawab commented Nov 1, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024

mlange05 left a comment

mlange05 Nov 1, 2024

reuterbal Nov 4, 2024

mlange05 Nov 4, 2024

reuterbal left a comment

reuterbal Nov 4, 2024

awnawab Nov 4, 2024

reuterbal Nov 4, 2024

reuterbal Nov 4, 2024

reuterbal Nov 4, 2024

reuterbal Nov 4, 2024

	if (builddir := kwargs.get('build_args', {}).get('builddir', None)) is not None:
	build_args = kwargs.get('build_args')
	if build_args and (builddir := build_args.get('builddir')):

Make builddir a runtime argument of FileWriteTransformation #425

Make builddir a runtime argument of FileWriteTransformation #425

Conversation

awnawab commented Nov 1, 2024

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 1, 2024

mlange05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reuterbal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make builddir a runtime argument of `FileWriteTransformation` #425

Make builddir a runtime argument of `FileWriteTransformation` #425

codecov bot commented Nov 1, 2024 •

edited

Loading