Repo reorganisation: Moving transformations #296

mlange05 · 2024-04-23T13:28:50Z

The next step in our spring clean (issue #253) is to move all the internally support transformations into the core package, alongside the associated tests. This then also merges the loki.transform general utilities into the more custom loki.transformations packages.

I've tried to restrain myself to only move files and fix-up import headers, with a few small exceptions listed in the details below:

Transformation and Pipeline class have been moved into loki.batch as they are the interface to the scheduler
transformations has been moved into loki.transformations
- the associated test have been moved in the same fashion
- the additional conftest.py has been resolved and the runner utilities for CLOUDSC and ecWAM added to loki.tools
The subpackages loki.transformations.single_column, loki.transformations.transpile and loki.transformations.build_system have been separated
- DependencyTransformation and ModuleWrapTransformation have been mode to separate files inside trnasformations.build_system
- CI And packaging has been changes as needed (please double-check this)
- loki.transform has been moved into loki.transformations, often removing the transform_xxx prefix

And finally, apologies to @MichaelSt98 , as this one might conflict quite a bit with his current stuff... 😬

github-actions · 2024-04-23T13:55:08Z

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

codecov · 2024-04-25T07:48:12Z

Codecov Report

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

Project coverage is 95.09%. Comparing base (102dafd) to head (fb56455).

Files	Patch %	Lines
loki/tools/files.py	15.78%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
+ Coverage   94.97%   95.09%   +0.12%     
==========================================
  Files         151      165      +14     
  Lines       32407    35282    +2875     
==========================================
+ Hits        30779    33552    +2773     
- Misses       1628     1730     +102

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`95.07% <95.71%> (-0.07%)`	⬇️
transformations	`?`

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.

MichaelSt98 · 2024-04-29T07:27:18Z

Thanks, really nice! I'm happy with it and haven't discovered anything problematic.
Nevertheless some comments:

I think in order to be consistent:

transformations/build_system/tests/test_transform_dependency.py --> transformations/build_system/test_dependency.py
transformations/tests/test_transform_derived_types.py --> transformations/tests/test_derived_types.py

Why not?

transformations/transform_loop.py --> transformations/loop.py
transformations/transform_region.py --> transfrormations/region.py

Me thinking out loud:

introduce subdirectories within transformations ?
- transformations/temporaries/ (or something like that) for hoist-variables, pool_allocator, ...
- transformations/control_flow/ (or something like that) for loop, inline, region, ...
- transformations/utilities/ for utilities, remove_code, drhook, ...
it makes sense for transformation and pipeline to be within batch as they are interfacing the scheduler, however, I somehow would have expected them to be in transformations/

I guess that further repo reorganisation like "separate operations (inlinecall, subscript, sum, ...) from the symbols/literals" will be done in a future/separate PR?!

mlange05 · 2024-04-29T12:50:00Z

@MichaelSt98 Many thanks for the review - to answer your questions:
1.

transformations/build_system/tests/test_transform_dependency.py --> transformations/build_system/test_dependency.py
transformations/tests/test_transform_derived_types.py --> transformations/tests/test_derived_types.py

Yes, absolutely, that was an oversight! Will fix in the needed rebase!

transformations/transform_loop.py --> transformations/loop.py
transformations/transform_region.py --> transfrormations/region.py

Mostly because I actually thought "transform" to be a good high-level description for "fusion/fission/interchange" and "hoist/extract". Open for suggestions though, as the grouping by target "node type" seemed not scalable to me.

Agree in general on the proposed further sub-division or the transformations package; this will potentially also require some re-jigging of the stuff insied some of those transformation (eg. lots in SCCBase could go into utility) and I simply wanted to keep this PR manageable.

With regards to Transformation/Pipeline moving, this simply creates a clean import chain, as everything in loki.transformations should be "do something to code", and thus depend on the ir/expression and batch sub-packages. Hope this makes sense. 😉

reuterbal

Sorry for taking so long to review this and many thanks, this looks fantastic.

I agree with all what's been said so far. Regarding the renaming to just loop & co I share your assessment that this might not scale well and the transform adds information despite being somewhat redundant at first look - since it is the commonly used terminology for the kind of changes applied by the utilities in there.

I've added some comments where I spotted missing docstrings or possible clean-up opportunities, but overall this looks very good.

The documentation scripts need to be updated, too, but since we have already dangling pointers in there and Sphinx steps over this gracefully, we may as well do a sweep in a separate PR to rectify this.

reuterbal · 2024-05-03T07:11:08Z

install

@@ -508,5 +507,5 @@ echo "Activate the Loki environment with"
 echo
 echo "    source loki-activate"
 echo
-echo "You can test the Loki installation by running 'py.test transformations lint_rules .'"
+echo "You can test the Loki installation by running 'py.test lint_rules .'"


This should be running 'py.test lint_rules loki', I think.

reuterbal · 2024-05-03T07:14:31Z

loki/transformations/build_system/module_wrap.py

+        """
+        Update imports of wrapped subroutines.
+        """
+        from loki.batch import SchedulerConfig  # pylint: disable=import-outside-toplevel,cyclic-import


This import could now also take place at the toplevel, no?

reuterbal · 2024-05-03T07:55:55Z

loki/transformations/tests/test_array_indexing.py



-@pytest.fixture(scope='module', name='here')
+@pytest.fixture(scope='function', name='here')


Not that it matters too much, but curious why the scope has changed for this?

Nice catch! I had previously used this in the build, but now it works without this, so can go back to "module" scoping.

reuterbal · 2024-05-03T07:57:24Z

loki/transformations/tests/test_array_indexing.py

    builder.clean()
-    (here/f'{driver.name}.f90').unlink()
-    (here/f'{kernel_module.name}.f90').unlink()
+    (tempdir/f'{driver.name}.f90').unlink()
+    (tempdir/f'{kernel_module.name}.f90').unlink()


This should be redundant with the tempdir clean-up in the fixture?

reuterbal · 2024-05-03T08:16:08Z

loki/tools/files.py

+def write_env_launch_script(here, binary, args):
+    # Write a script to source env.sh and launch the binary


It took me a while to figure out why we have these because they didn't show up with test coverage. So, I would suggest adding short docstrings:

Suggested change

def write_env_launch_script(here, binary, args):

# Write a script to source env.sh and launch the binary

def write_env_launch_script(here, binary, args):

"""

Utility method that is used for regression tests that require

activating an environment file before running :data:`binary`.

This writes a simple script of the form

.. code-block::

source env.sh

bin/<binary> <args>

exit $?

Parameters

----------

here : pathlib.Path or str

The directory in which the script is created

binary : str

The name of the binary

args : list of str

List of arguments to pass to the binary

Returns

-------

pathlib.Path

The path to the created script file

"""

Thanks for these! Added as suggested.

loki/tools/files.py

reuterbal · 2024-05-03T08:34:49Z

loki/tools/files.py

+    return str(lokidir.resolve()), target, backup
+
+
+def local_loki_cleanup(target, backup):


Suggested change

def local_loki_cleanup(target, backup):

def local_loki_cleanup(target, backup):

Companion utility to :any:`local_loki_setup` to revert the changes

This removes a symlink at :data:`target`, if it exists, and moves the

:data:`backup` path in its original location.

Parameters

---------

target : pathlib.Path

The target injection path as returned by :any:`local_loki_setup`

backup : pathlib.Path

The backup path as created by :any:`local_loki_setup`

This also split the module_wrap transformation from the dependency transformation and puts it into its own source file.

mlange05 · 2024-05-03T12:35:03Z

Ok, apologies, this took a bit longer than anticipated. @reuterbal I think I've addressed all comments - many thanks for the suggestions!

I've also tried to address the naming inconsistencies pointed out by @MichaelSt98, but those generated some name clashes in the test base (test_derived_type existed twice). So instead I've opted to rename loki.transformations.derived_type to loki.transformations.transform_derived_types to match the test. I hope this works now, and I think we'll get to tidy this up a little further in a follow-on PR.

reuterbal

Thanks for this. I found another oopsie and my docstring suggestion was faulty (sorry!), but otherwise good to go!

reuterbal · 2024-05-03T14:15:50Z

install

@@ -512,7 +511,7 @@ print_step "Installation finished"
 echo
 echo "Activate the Loki environment with"
 echo
-echo "    source loki-activate"
+echo "    source loki-activate"xf


Something's gone wrong here

Suggested change

echo " source loki-activate"xf

echo " source loki-activate"

reuterbal · 2024-05-03T14:18:44Z

loki/tools/files.py

+    """
+    Utility method that is used to determine paths for injecting the
+    currently running source code of Loki into an
+    [ecbundle](https://github.com/ecmwf/ecbundle)-based worktree This


Sorry, got caught out by the annoying RST vs. MD syntax differences for link formatting...

Also, empty line after the first sentence improves formatting in the HTML output.

Suggested change

[ecbundle](https://github.com/ecmwf/ecbundle)-based worktree This

`ecbundle <https://github.com/ecmwf/ecbundle>`_-based worktree.

This

mlange05 requested review from reuterbal and MichaelSt98 April 23, 2024 13:28

mlange05 force-pushed the naml-repo-reorg-trafo branch from ce687fa to 048c4b5 Compare April 23, 2024 13:52

mlange05 force-pushed the naml-repo-reorg-trafo branch 7 times, most recently from 3bcad1b to 7f70cc3 Compare April 25, 2024 07:15

mlange05 force-pushed the naml-repo-reorg-trafo branch from 7f70cc3 to 11ec7c1 Compare April 25, 2024 07:52

mlange05 marked this pull request as ready for review April 25, 2024 08:34

reuterbal approved these changes May 3, 2024

View reviewed changes

mlange05 added 10 commits May 3, 2024 09:27

Batch: Move Transformation and Pipeline into loki.batch

7d64963

Transformations: Moving the transformations package inside loki

294c246

Transformations: Adjust CI to new loki subpackages

2ea7749

Transformations: Remove conftest and move utilities to loki.tools

703d703

Transformations: Move single_column transformations into sub-package

6b8753c

Transformations: Move transpile transformations into sub-package

6350fa1

Transformations: Move build system transformations into sub-package

8306b2d

This also split the module_wrap transformation from the dependency transformation and puts it into its own source file.

Transformations: Moving remaining high-level transformations over

5de0b0e

Transformations: Fixing imports to appease linter gods

13be40f

Transformations: Add test-specific build dir to test to avoid aliasing

bde9395

mlange05 force-pushed the naml-repo-reorg-trafo branch 3 times, most recently from 0dbb754 to da4814c Compare May 3, 2024 11:29

mlange05 force-pushed the naml-repo-reorg-trafo branch from da4814c to fddecf8 Compare May 3, 2024 11:48

reuterbal approved these changes May 3, 2024

View reviewed changes

mlange05 added 3 commits May 3, 2024 14:44

Tools: Add docstrings for env and bundle handling

dcc02de

Loki: Fix imports, fixture scoping and install script comment

b915b42

Transformations: Rename build system test and derived type transform

fb56455

mlange05 force-pushed the naml-repo-reorg-trafo branch from fddecf8 to fb56455 Compare May 3, 2024 14:44

mlange05 merged commit dd01f94 into main May 3, 2024
12 checks passed

mlange05 deleted the naml-repo-reorg-trafo branch May 3, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repo reorganisation: Moving transformations #296

Repo reorganisation: Moving transformations #296

mlange05 commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024

codecov bot commented Apr 25, 2024 •

edited

Loading

MichaelSt98 commented Apr 29, 2024

mlange05 commented Apr 29, 2024

reuterbal left a comment

reuterbal May 3, 2024

reuterbal May 3, 2024

reuterbal May 3, 2024

mlange05 May 3, 2024

reuterbal May 3, 2024

reuterbal May 3, 2024

mlange05 May 3, 2024

reuterbal May 3, 2024

mlange05 commented May 3, 2024

reuterbal left a comment

reuterbal May 3, 2024

reuterbal May 3, 2024



		@pytest.fixture(scope='module', name='here')
		@pytest.fixture(scope='function', name='here')

		def write_env_launch_script(here, binary, args):
		# Write a script to source env.sh and launch the binary

-def write_env_launch_script(here, binary, args):
-    # Write a script to source env.sh and launch the binary
+def write_env_launch_script(here, binary, args):
+    """
+    Utility method that is used for regression tests that require
+    activating an environment file before running :data:`binary`.
+    This writes a simple script of the form
+    .. code-block::
+       source env.sh
+       bin/<binary> <args>
+       exit $?
+    Parameters
+    ----------
+    here : pathlib.Path or str
+        The directory in which the script is created
+    binary : str
+        The name of the binary
+    args : list of str
+        List of arguments to pass to the binary
+    Returns
+    -------
+    pathlib.Path
+        The path to the created script file
+    """

		return str(lokidir.resolve()), target, backup


		def local_loki_cleanup(target, backup):

-def local_loki_cleanup(target, backup):
+def local_loki_cleanup(target, backup):
+    Companion utility to :any:`local_loki_setup` to revert the changes
+    This removes a symlink at :data:`target`, if it exists, and moves the
+    :data:`backup` path in its original location.
+    Parameters
+    ---------
+    target : pathlib.Path
+        The target injection path as returned by :any:`local_loki_setup`
+    backup : pathlib.Path
+        The backup path as created by :any:`local_loki_setup`

Repo reorganisation: Moving transformations #296

Repo reorganisation: Moving transformations #296

Conversation

mlange05 commented Apr 23, 2024 • edited Loading

github-actions bot commented Apr 23, 2024

codecov bot commented Apr 25, 2024 • edited Loading

Codecov Report

MichaelSt98 commented Apr 29, 2024

mlange05 commented Apr 29, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlange05 commented May 3, 2024

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

mlange05 commented Apr 23, 2024 •

edited

Loading

codecov bot commented Apr 25, 2024 •

edited

Loading