Allowing file-specific transformation scripts for PSyclone

[jasonjunweilyu]

This pull request is related to Issue https://github.com/hiker/fab/issues/18 and the pull request on hiker fab: hiker#19. The purpose is to allow each x90 file to have its specific transformation script.

(1) The changes are mainly in:

source/fab/steps/psyclone.py: The psyclone function now takes in a function as the argument for the transformation_script parameter. This input function will be written by the user to take in an x90 file path (parameter fpath) and the config, and return a transformation script path.

(2) The related tests have also been updated:

tests/system_tests/psyclone/test_psyclone_system_test.py: Tests have been added to verify that the input transformation script function takes in the x90 file path and returns a transformation script path, which is then passed successfully to the psyclone command with -s. The checking for the transformation_script_hash is now done in the unit test, as now the transformation_script_hash is calculated when processing each file.

tests/unit_tests/steps/test_psyclone_unit_test.py: Transformation_script_hash test and prebuild_hash test have been updated.

(3) Documentations and examples have been updated:

docs/source/writing_config.rst: A PSyclone step is added to this config writing documentation to demonstrate how to use this step.

run_configs/lfric/atm.py and run_configs/lfric/gungho.py: The atm and gungho examples have been updated to show examples of how a get_transformation_script function can be defined and then passed to psyclone.

[hiker]

I've had a look through this change and the goal of providing special cases to the transformation script is certainly a functional requirement so I have no problem with that.

Thanks!

My question is around the implementation. Why a callback function? It seems to add complexity and I'm not sure what's being won by its use.

Why not pass a path to a global script (optional because we may not want transformations) and a dictionary of special cases (also optional because we may have none).

This implementation allows the same usage, just passing in the global script, so we are not losing anything. A dictionary could be useful, but it can also be used in the function being provided. So, functionality-wise this approach can support a dictionary of special cases.

But, if a Fab script uses this dictionary, it means that adding a new file-specific PSyclone script to the optimisation directory in LFRic would mean that we then need to modify the Fab script as well (adding this new script to the dictionary), while it is not necessary with the example functions provided - the function will dynamically detect any scripts as required, so we have the same behaviour as the current LFRic build system.

Providing a function means any typo in the function name is discovered much earlier, either when you import the function, or when you call the PSyclone step ... way before it would be called. And typing means we have a proper typed variable (as opposed to a string or path, which can be a function taking any kind of parameters). And the implementation is much more straightforward (no importlib or exec or import or whatever you use). Also, syntax errors in the script are detected much earlier (immediately when importing, not deep down the fab call tree).

The LFRic Fab scripts are based on an OO-design - which means the base class will provide the same behaviour the old build-systems has (checking for a python script with the same local path and base name as the x90 file), and all application-specific build scripts don't need to deal with this at all - the base class will set its own method to be used. So in LFRic a typical user script will never see this function.

The only reason I can think of for the callback is if the mapping it performs is contingent on the configuration and off the top of my head I can't think of a situation where that would be so.

The config object is required to properly create relative path names (in case that more directory levels are added to some of the more complex LFRic apps). And we also intend at some stage that the config object stores information whether OpenMP is enabled or not. This is required, since Fab will have to trigger fparser to handle or ignore omp conditional sentinels in any source code during analysis - and it will then also be able to enable/disable this in the (new and upcoming) compiler objects independent of the compiler. Meaning the configuration will be simpler. Which also means, a PSyclone scripts can query the config object for OpenMP, and if it is disabled not apply any OpenMP transformations at all.

[hiker]

Actually, there is one additional reason why I would prefer more flexibility: as part of NG-ARCH (UM-physics to non-CPU architectures), we will be using PSyclone on UM files (i.e. f90 files). I am not really certain at this stage what kind of scripting we will end up with (one script for all of UM?? Unlikely, but who knows). Same approach as lfric? Again, having a function will give us the required flexibility

[jasonjunweilyu]

Hi @MatthewHambley , thanks for the review. I would second @hiker's opinions. For me, implementing this with a callback function would have the following benefits:

(1) Matching the current LFRic build system behaviours: LFRic_atm currently has two file-specific optimisation scripts that uses the mapping from .x90 files to .py files: https://code.metoffice.gov.uk/trac/lfric_apps/browser/main/trunk/applications/lfric_atm/optimisation/meto-xc40/algorithm

(2) Allowing flexibility for users without needs to touch the FAB repo: With the callback function, users can define their own logics for getting optimisation scripts by simply changing this function in the app build scripts managed by themselves. They can choose to just use the mapping method given in the example or use a dictionary for matching optimisation scripts to x90 files or use other logics for future applications.

(3) Fitting into the LFRic Fab OO-design scripts: This callback function will be a method of the LFRic Fab OO build script base class, along with several other methods. So it won't be too special or complex for users. Users can choose to just use it by default without touching it or overwrite it in the app-specific build scripts.

[MatthewHambley]

I must be missing something because I don't see how the callback you are describing provides any more functionality.

The logic for compiling the list of optimisation functions must exist somewhere and I don't see how, functionally, it makes any difference whether the list is compiled dynamically as need (callback) or up front at initialisation. Unless you are worried that it will get very large and become a burden on memory.

Ignoring what happens in the Fab framework and looking just at the user's build script, this is what I think this change provides:

def discover_transformation_script(algorithm: Path) -> Path:
    Synthesize transformation script path
    If file exists, return it, otherwise return global script.
...
fab_psyclone_step(discover_transformation_script)

And this is what I'm suggesting:

def discover_transformation_scripts(root: Path) -> Dict[Path, Path]:
    Iterate over transformation scripts:
        Add mapping from algorithm to transformation
    return mappings
...
fab_psyclone_step(global_transformation_script_path, discover_transformation_scripts(...)

I don't see any difference in where (who has responsibility) the dynamic content is being generated.

So what have I missed. What does the callback give that a static look-up table doesn't? Both generate their data dynamically and client side. Neither require modifications to the Fab framework when a new transformation is added.

[hiker]

OK, I understand you know, I thought you were talking of a static mapping.

I had a chat with Jason today. The two approaches seems to be identical in what can be done with them, with slightly different run time behaviour, which will de-facto not matter (you call the function once in serial, but need to traverse the whole directory. The approach her calls it for each algorithm file, but in parallel and only checks for existence of two files). All other functionality that one offers can be done with the others as far as we can see.

So the main question is: is there anything wrong with this approach?

[MatthewHambley]

Having thought about it some more and conferred with colleagues I have been won round to the callback approach. So I will proceed with the review on that basis.