Added support for providing multiple scripts with psyclone

[schreiberx]

No description provided.

[sergisiso]

I am happy to review this one (once the tests pass) but @arporter @LonelyCat124 @hiker haven't we discussed this one before (having multiple -s script1.py -s script2.py)? I can't find an issue but I would have sworn we had it and we agreed it was a good idea.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (34206b0) to head (49e0e39).
⚠️ Report is 179 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3594   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          401       401           
  Lines        56540     56624   +84     
=========================================
+ Hits         56540     56624   +84     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hiker]

I am happy to review this one (once the tests pass) but @arporter @LonelyCat124 @hiker haven't we discussed this one before (having multiple -s script1.py -s script2.py)? I can't find an issue but I would have sworn we had it and we agreed it was a good idea.

Not that I remember. I am a big fan of using more OO approach (i.e. one script inheriting from another) - which we can do already (i.e. trans just creating an instance ...), and we have discussed hierarchical search paths in the context of the build system (e.g. when we decided not to integrate the psyclone yaml specification from lfric).

What is the use case of multiple script - especially what can be achieved with that that we can't already do? FWIW, I already have an example in my tutorial (e.g. tutorial/training/gocean/2.10-GameOfLife-mpi/solution/openmp.py) where I use one transformation script in another:

from copy_kernels_and_fuse_loops import trans as fuse_trans
...
def trans(psyir: FileContainer)
...
    # Call the existing fuse transformation, which will also module inline.
    fuse_trans(psyir)
...

Together with arguments for scripts (which can e.g. be used to specify which scripts to tun), I can't see anything that multiple scripts can do that we can't already do, or am I missing something?

I don't really object, but am a big fan of KISS :)

[hiker]

And I just notice, how does this work together with keyword arguments? I assume they would be provided for both scripts?

[hiker]

And I just notice, how does this work together with keyword arguments? I assume they would be provided for both scripts?

Ah, I had a quick look now (sorry, should have checked before writing) - each script get its own kwargs.

[schreiberx]

I am happy to review this one (once the tests pass) but @arporter @LonelyCat124 @hiker haven't we discussed this one before (having multiple -s script1.py -s script2.py)? I can't find an issue but I would have sworn we had it and we agreed it was a good idea.

Not that I remember. I am a big fan of using more OO approach (i.e. one script inheriting from another) - which we can do already (i.e. trans just creating an instance ...), and we have discussed hierarchical search paths in the context of the build system (e.g. when we decided not to integrate the psyclone yaml specification from lfric).

What is the use case of multiple script - especially what can be achieved with that that we can't already do? FWIW, I already have an example in my tutorial (e.g. tutorial/training/gocean/2.10-GameOfLife-mpi/solution/openmp.py) where I use one transformation script in another:

from copy_kernels_and_fuse_loops import trans as fuse_trans
...
def trans(psyir: FileContainer)
...
    # Call the existing fuse transformation, which will also module inline.
    fuse_trans(psyir)
...

Together with arguments for scripts (which can e.g. be used to specify which scripts to tun), I can't see anything that multiple scripts can do that we can't already do, or am I missing something?

I don't really object, but am a big fan of KISS :)

You're asking why this would be nice? Here we go:

1. I would first like to use Poseidon to rewrite the code.
2. Then I like to use, e.g., the OpenACC or OpenMP backend of PSyclone.
3. Maybe I would like to do some further injection of debugging features in between or after it.

For this, I don't like to mess around with calling scripts from other scripts, but just pipeline all of them on the command line.

[sergisiso]

Another reason is that scripts are not always properly packaged to guarantee that imports always work.

This is not now we use NEMO, but there is a way to let the NEMO build system call psyclone that copies the scripts from the source directory to a build directory with a different structure. Currently any "imports" in those scripts become broken. You could say that it is the build system problem, but the problem also goes away if they are "independent" scripts that are applied one after the other.

In any case this doesn't remove the current functionality, it just provides an alternative.

[sergisiso]

@schreiberx Have a look at the coverage misses before I do the review.

We can also have a discussion in the next telco to make sure everybody agrees.

[hiker]

You're asking why this would be nice? Here we go:

1. I would first like to use Poseidon to rewrite the code.
2. Then I like to use, e.g., the OpenACC or OpenMP backend of PSyclone.
3. Maybe I would like to do some further injection of debugging features in between or after it.

For this, I don't like to mess around with calling scripts from other scripts, but just pipeline all of them on the command line.

One alternative to a PR:

#!/usr/bin/env python3

import sys
from psyclone.generator import load_script

def trans(psyir: FileContainer, scripts: list[str]):
    for script in scripts:
        print("LOADING", script)
        transformation_recipe, _, _, _ = load_script(script)
        transformation_recipe(psyir)
        del sys.path[0]

Usage:

psyclone  ... -s ./multi_script.py   --script-kwargs scripts:'["copy_kernels_and_fuse_loops.py", "openmp.py"]'
LOADING copy_kernels_and_fuse_loops.py
LOADING openmp.py

Though this of course needs some additional work to support different command line options per script, and to use the files_to_skip option.

TBH, in your case, I would just use command line options ("parallel='openmp',debug=['phase1', 'phase3']").

Re Sergi's comment about the old-style(?) NEMO build system moving files around: since this is using the PSyclone loader, I think this should work with whatever NEMO is doing there.

I don't object to adding this, but I feel that PSyclone is moving away from it's core ability, and instead handles issues with build systems, script design etc. (and the above del sys.path[0] actually shows why load_script needs some refactoring ;) ).

[sergisiso]

What do @arporter @LonelyCat124 think about this? I still find it relatively natural that a psyclone command accepts multiple -s.

@schreiberx We still need some clarity/tests about what happens with global script options: https://psyclone.readthedocs.io/en/latest/user_guide/user_scripts.html#script-global-variables
Imo, FILES_TO_SKIP should only apply to the script that have them, but RESOLVE_IMPORTS (which affects the parsing) should accomulate, with True having priority above others, and otherwise aggregating a set of all modules listed in any scripts.

[schreiberx]

I think that neither FILES_TO_SKIP, nor RESOLVE_IMPORTS should be used here. This should be part of the transformation script. But that should be part of a different PR. As an interim solution, they should all be accumulated until a better solution is found.

E.g., to avoid this, a script could look like this:

def trans(psyir, control: PsycloneControl):
    # Could also be done through .ancector(FileContainer)
    if control.get_filename() in ["asdf.f90", "asdf.f90"]:
        return        # Skip processing this file

    # Resolve all imports for the current psyir file
    control.resolve_imports(psyir)

[sergisiso]

@schreiberx This is not possible:

if control.get_filename() in ["asdf.f90", "asdf.f90"]:
return # Skip processing this file

This is already done when the issue is the script, the FILES_TO_SKIP is to skip files where the issue is in the frontend or backend, so an empty passthrough (skipping the script) also generated invalid code.

#Resolve all imports for the current psyir file
control.resolve_imports(psyir)

This works but it is much slower (because changing symbol information after parsing needs to traverse the tree updating some nodes with the new information to keep things consistent), also some logic during the frontend, e.g. resolving WHERE constructs is worse without the symbol information.

[schreiberx]

OK. This now makes all sense to me.

Then, concatenating all FILES_TO_SKIP and OR-ing RESOLVE_IMPORTS would be the approach we'd go for?

[sergisiso]

Then, concatenating all FILES_TO_SKIP and OR-ing RESOLVE_IMPORTS would be the approach we'd go for?

Yes for Or-ing (or Union-ing is Set notation terms), with a 'True' taking precedence for RESOLVE_IMPORTS.

[schreiberx]

I think we're ready to go.

[sergisiso]

@schreiberx As I mentioned I'm fine with the overall idea of providing multiple scripts to a psyclone command, but the current implementation have some problems that need to be fixed, see inline.

[sergisiso]

This is not sufficient. For -s first.py -s second.py --script-kwargs 'x: 1', this assigns x to the first script rather than the most recent script, contrary to the documented behaviour.

Add a test about this.

We also want to check that something like:
-s first.py --script-kwargs 'x: 1' --script-kwargs 'x: 2' -s second.py is invalid instead of silently overwritten/misaligned

[sergisiso]

Remove the previous version of this at line 1059

[sergisiso]

In fact this changes the behaviour, if we have:

folder1/script.py
    def trans():
        import utils
folder1/utils.py
folder2/script.py
    def trans():
        import utils
folder2/utils.py

And we do
psyclone -s folder1/script.py -s folder2/script.py , now the utils.py won't be found because it is a runtime import which only happens when the trans_recipe(psyir, **kwargs) is called below.

You will need to restructure all this section so the the sys.path.remove(inserted_path) happens after trans_recipe but still before the load_script of the next script is executed, otherwise it gets the wrong path for that (e.g. it would impor the wrong utils in the example above)

[sergisiso]

That's wrong, here we load all scripts and just rewrite the final_resolve_mods. When two recipes specify different RESOLVE_IMPORTS lists, only the last nonempty list reaches FortranReader, so the first recipe no longer receives the resolved symbols it requested.

I thought we said we would be "or-ing"/"union-ing" the requested RESOLVE_IMPORTS, and let a "True" take precedence about everything else.

[schreiberx]

@sergisiso It seems that there's no way to relate some 'script-arguments' argument following a 'script' argument with argparse. I couldn't find a way to determine the argument's position, and I think mixing it up with sys.argv parsing isn't good.

How about this solution that is also backward compatible:

• If no script argument is provided, that's all right.
• As soon as script arguments are provided, they must match the number of total scripts provided. In this way, they can be again directly related to each other.

[sergisiso]

@schreiberx Let's revert to just accepting one set of script-kargs and provide the same set to all scripts. A script is free to ignore kwargs anyway.

[schreiberx]

@schreiberx Let's revert to just accepting one set of script-kargs and provide the same set to all scripts. A script is free to ignore kwargs anyway.

I first thought that it was a good idea since it might look like some over-engineering, but then I thought about use cases similar to what I have in my other development:
There are arguments to operations that filter out processing for some subroutines in case of a problem.
This must be operation-specific, or in our case, specific to the script.
I could make up even more examples :-).