3610 file path for file container - #3621

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6eefb42) to head (ea1ef1a).
⚠️ Report is 14 commits behind head on master.

@@            Coverage Diff            @@
##            master     #3621   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          403       403           
  Lines        56802     56833   +31     
=========================================
+ Hits         56802     56833   +31     

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

Reasonable small change to add a file_path attribute to FileContainer. To avoid passing down the path across many levels of parsing, in most cases (the ones a user script will typically see) the file_path is set after the parsing is done, before the PSyIR is passed to a user transformation function. I did add support to the FileInfo class (which the module manager uses), so this also means that any PSyIR from the module manager will have this attribute set as expected.

The two 'odd; ones:

• the psy-layer which will get the path set later (before calling user script)
• the alg layer - since it starts as the input file, and is then 'transmuted' into the alg layer. So, before calling the user trans_alg function, I set the future name of the alg layer (based on --alg).

I did update some files to start using pathlib, which makes some changes look much bigger than they actually are.

I have confirmed that this feature works for writing out GPU markup information (separate PR incoming for that :) )

IT passed, ready for review

Thanks very much Joerg. I like the basic functionality and think it all makes sense. However, I'm not keen on then using that functionality to set the name of the file which the PSyIR will be written to. That information does not belong in the PSyIR as the writing of code to file lives outside. How does this play into your work towards supporting GPU markup?

OK, the way to planned mark up works is that a PSy-layer file X is given to a transformation script, which calls a (PSyclone) function that will write the static call tree (and symbols used) into a file somewhere (=X.calltree.yaml or so), making sure not overwriting stuff a parallel process might be using (unless we use locks for a shared file). Then in the next PSyclone phase, we find all *calltree.yaml files, merge the information (modules might be used in several yaml files with potentially different functions called), and then (in parallel) mark up all functions/symbols required.

So, the problem is where is X :) Options:

1. 'next' to the PSy-layer file. For this, we need the location of PSy-layer file, and change the suffix. Advantage: all the directory managing/cleanup is already done anyway by the build system. But, atm we can't get X.
   1. We get it from the only guaranteed input to a function, the FileContainer, i.e. this patch. Which you don't like :)
   2. We let the function that computes the call tree and writes the files decide where to place the file (it would then need a way to get the argument list, which can be done). This would somewhat limit the flexibility of the script and build system where files should be stored (imho, the build system/user script should be able to control this, but - I actually can't think of a situation where this would be a problem).
   3. We provide a function in PSyclone that a user script can call to get the output file name. Again, possible - though it doesn't feel right.
   4. We provide the name as additional argument? That would break all existing PSyclone scripts (unless we analyse the signature of trans to see if the script accepts an additional argument). Doesn't feel right.
2. The build system provides a flat directory (or a mirror of the user's directory structure), where all .calltree.yaml files are written to, and we either hard-code that path in the user script, or provide this directory as keyword argument (and the build system would know which trans-function would need that info ... somehow). That would work, it means minor work on the build system to create and manage/clean (if required) this directory.
3. Some demon/DB running that can be queried :) Too complicated imho.

IMHO, a FileContainer having a file_path would match its functionality, and explain the difference between Container and FileContainer (atm there is none). Also, the name property of the container already is the base-filename if I am not mistaken, just no paths or suffix)

I do get the issue that for an existing file file_path makes more sense than for a psy-layer (and/or alg layer) file, which does not yet exist. @AidanChalk , @sergisiso - opinions? FWIW, I like my solution (surprise ;) ), but am happy to go with a build-system controlled directory (which is what I am already using ;) ). Or a function in PSyclone to query the current psy-layer-output-filename.

If we agree to not have the psy-layer name in the FileContainer, I would then actually consider not adding this attribute at all. If a user script can't rely on this attribute in all cases (i.e. it can if the PSyIR comes from an existing file, e.g. using the module manager, but not for alg- or psy-layer), it would either remained unused, or causes if-statements and some kind of code to handle if file_path is None in user transformation scripts. Might be more straight-forward to then just use a keyword argument.

The final argument for this attribute: in the future, additional applications might need this? Maybe ... a script want to cache (based on a hash of the file content) DUC information to speed up processing of files when nothing has changed? That cache could use the same file-path information. That's admittedly the only thing I could currently think of (any other cache information like Martin's suggested cached type information would be handled by PSyclone internally, so it can access the file name).

Then in the next PSyclone phase, we find all *calltree.yaml files, merge the information (modules might be used in several yaml files with potentially different functions called), and then (in parallel) mark up all functions/symbols require.

Is this a phase handled by the build system itself? I was hoping to do something agnostic to the build system, e.g. that it could also work with NEMO, current LFRic, whatever LFRic wants to do next.

What I had it mind was two* calls to the build system:

1. A script that processes psy-layer to do the call tree analysis of each kernel call and write an annotations.yaml (I am not sure if we need it separated by different files , e.g. X.* but this is a separate discussion). And I think the locking on that file while updating it is fine, I assume the analysis take much longer than the file update operation. This file would have a dictionary of 'module':'variable/subroutine to annotate'.
   Since build systems typically follows build dependencies, probably these modules that have already been psycloned/(compiled?) and its too late to modify them now.

2. A second call to the build system with the only change required to the build system is that all files need to go through psyclone (no need to change the order or add phases).
   This time the non psy-layer files have a script that would check the provided annotations.yaml if the module is listed there apply the requested annotation. The psy-layer file would do the same as our current gpu script, but allowing calls inside kernels as this should have been piked by the previous analysis and assume that now are annotated.

We don't need the path in this case* because we expect all files to go through psyclone during the second build, at some point all the 'module' in the yaml file will go through it.

(* technically you can have a top-level build target that does these 2 steps)
(* maybe there could be issues if we have the same module name in two files)

Then in the next PSyclone phase, we find all *calltree.yaml files, merge the information (modules might be used in several yaml files with potentially different functions called), and then (in parallel) mark up all functions/symbols require.

Is this a phase handled by the build system itself? I was hoping to do something agnostic to the build system, e.g. that it could also work with NEMO, current LFRic, whatever LFRic wants to do next.

Well, I suggested to include the psyclone control file (which can be used to specify several psyclone passes, and which I intend to extend to work with this setup) :)

But I can't really see how this can be done without involving the build system. A suite might be running transmute and DSL in any order (transmute, dsl, transmute) - only at the very end we will have the complete information what needs to be marked up (at least in theory a transmute step could read in a psy-layer file?? - Unlikely admittedly, but who knows).

I can't really see how PSyclone can know that it should run an additional step.

What I had it mind was two* calls to the build system:

1. A script that processes psy-layer to do the call tree analysis of each kernel call and write an annotations.yaml (I am not sure if we need it separated by different files , e.g. X.* but this is a separate discussion). And I think the locking on that file while updating it is fine, I assume the analysis take much longer than the file update operation. This file would have a dictionary of 'module':'variable/subroutine to annotate'.

I wouldn't agree with that. Yes, timing-wise you are right, but given your definition of the information, it means the file has combined the individual dependencies using the module as key. A module can be used by two different files A and B, each one calling a different subroutine subA and subB in the module. So, if A is modified to remove its call to subA ... can we then remove the dependency to subA from the module? No, because there might be any number of other functions calling subA. So, we would also need to store which file(s) are responsible for adding a dependency, and managing this information.

Also, if I am not mistaken, If we store the information per file in a central file, then we have overall O(n^2), since n files will need to sort out the information for n files when updating its information.

This is imho huge overhead for no gain. ATM, handling this is trivial: you take a dictionary[str, set[str]], use the module name as key, and add all functions to the set.

Individual files is definitely the KISS solution imho.

Since build systems typically follows build dependencies, probably these modules that have already been psycloned/(compiled?) and its too late to modify them now.

There can't be a 'too late' imho. If a file is updated adding a new dependency, the new dependency needs to be marked up. We should not rely on always enforcing a clean build. Though admittedly, I just realise that the markup step (which isn't fully done yet) should test if function/symbol isn't already marked up ;)

2. A second call to the build system with the only change required to the build system is that all files need to go through psyclone (no need to change the order or add phases).
   This time the non psy-layer files have a script that would check the provided annotations.yaml if the module is listed there apply the requested annotation. The psy-layer file would do the same as our current gpu script, but allowing calls inside kernels as this should have been piked by the previous analysis and assume that now are annotated.

Hmm - I wonder if I misunderstood something: yes, the build system needs to trigger an additional phase (if upmarking is required, e.g. not if you are just doing CPU openmp). Maybe I misunderstood your statement above that this should be agnostic to the build system.

We don't need the path in this case* because we expect all files to go through psyclone during the second build, at some point all the 'module' in the yaml file will go through it.

Agreed, at this stage we don't need the file_path attribute anymore

(* technically you can have a top-level build target that does these 2 steps) (* maybe there could be issues if we have the same module name in two files)

Same module name in two files is a nightmare for any build system :) Fab explicitly doesn't allow that. If there are two implementations (dl_esm_inf has two parallel_mod files or so, one with MPI, the other a dummy wrapper), you need to take care that the corresponding one is added to the list of files being used (and I believe that applies to amy build system. Symbol names are typically using the module name to distinguish them, so I would expect that this results in linking issues anyway)

I don't think we need to worry about the same module name in two files - such a thing cannot be compiled due to the namespace collision. e.g. there are indeed two parallel_mod in dl_esm_inf but only one of them is ever compiled into the library. This does imply that the application of PSyclone is driven by the build system and not just applied to every source file it can find.

Let me just try re-stating what I think it is we're trying to solve:

1. PSyKAl Kernels call other routines and some access global data (although my understanding is that this latter problem is gradually being fixed).
2. To offload such kernels, all of the routines they call must also be compiled for offload and therefore need to be marked up with a directive.
3. Similarly, any global data accessed must also be marked up with a directive where it is declared.

First off, I'm not convinced that we'll ever get good GPU performance if a kernel calls other routines - I would have thought that's too much code and stack for an efficient GPU kernel (i.e. we'll get very poor occupancy). However, we have to crawl before we can walk and to do that we do need a solution.

We could get PSyclone to chase down the targets of calls and mark them up. However, we know that having PSyclone modify files other than the one it has been given (or is generating) makes life very difficult and won't play nicely with parallel builds. So, we need a mechanism to mark-up the targets of calls and for that we need to know which ones to mark up. Therefore, we need:

1. a stage that gathers all of the dependencies of a Kernel/Routine (both global data and routines) and then collates that for all routines/kernels;
2. a second stage that attempts to add the necessary markup to all of those dependencies;
3. finally, the 'proper' PSyclone pass can chase down dependencies (again) and check that the necessary directives have been added (not all will have been) before it transforms the Kernel.

I'm worried about stage 2 - what happens when the "necessary markup" is more than "compile this for GPU" - i.e. if there's parallelism inside the routine that we want to exploit? Or is the assumption that we're at the bottom of the stack and there's no parallelism we want?

Ideally, we'll have at least module-inlined as many routines as possible which will actually remove them as a dependency for a given call site. I guess that has to happen as part of stage 1?

I don't think we need to worry about the same module name in two files - such a thing cannot be compiled due to the namespace collision. e.g. there are indeed two parallel_mod in dl_esm_inf but only one of them is ever compiled into the library. This does imply that the application of PSyclone is driven by the build system and not just applied to every source file it can find.

Agreed.

Let me just try re-stating what I think it is we're trying to solve:

1. PSyKAl Kernels call other routines and some access global data (although my understanding is that this latter problem is gradually being fixed).
2. To offload such kernels, all of the routines they call must also be compiled for offload and therefore need to be marked up with a directive.
3. Similarly, any global data accessed must also be marked up with a directive where it is declared.

First off, I'm not convinced that we'll ever get good GPU performance if a kernel calls other routines - I would have thought that's too much code and stack for an efficient GPU kernel (i.e. we'll get very poor occupancy). However, we have to crawl before we can walk and to do that we do need a solution.

I can't comment om the performance, not enough experience. 'Pseudo'-dead-code elimination (based on known namelist file settings to e.g. cut down the call tree in jacobian or so) will help with that.

I am also not entirely certain about no kernel using module variables - maybe? TBH, from a software engineering point of view using e.g. a module with global and common constants (planet radius) makes sense, and it feels very inconvenient to pass all potential variables through (I understand that this is the original design, but maybe not the reality). Hmm - pseudo-constant-replacement might also help with that (i.e. we read namelist file, and hard-code values :) ).

But agree with crawling :)

We could get PSyclone to chase down the targets of calls and mark them up. However, we know that having PSyclone modify files other than the one it has been given (or is generating) makes life very difficult and won't play nicely with parallel builds. So, we need a mechanism to mark-up the targets of calls and for that we need to know which ones to mark up. Therefore, we need:

1. a stage that gathers all of the dependencies of a Kernel/Routine (both global data and routines) and then collates that for all routines/kernels;

Ah yes. I always assumed that development iterates, i.e. if we build repeats and we manually exclude files to be compiled for GPU (because or problems elsewhere).

Advantage: we notice if something breaks. Disadvantage: manual work.

Your proposal of course automates it all, which is quite nice, but we need to find a way of tracking that we not suddenly lose kernels that were on GPU before.

Both approaches can probably be done, depending on developer's preference?

But I like this idea.

2. a second stage that attempts to add the necessary markup to all of those dependencies;

Yes, but that might be third or fourth step :) There is at least one app that transmutes a kernel, so that would be transmute, then DSL, then transmute for marking up. I think we should stay flexible with the number of passes.

3. finally, the 'proper' PSyclone pass can chase down dependencies (again) and check that the necessary directives have been added (not all will have been) before it transforms the Kernel.

Actually, if we have file-specific dependency files (as opposed to a global file as discussed with Sergi), there is no need to do that again, we can just read the corresponding yaml file and have this information (we might even want to store the file path into the yaml file as well. The module manager should always find the files for us, but that's faster, and cheap storage wise).

I'm worried about stage 2 - what happens when the "necessary markup" is more than "compile this for GPU" - i.e. if there's parallelism inside the routine that we want to exploit? Or is the assumption that we're at the bottom of the stack and there's no parallelism we want?

I don't see a problem with that from a build point of view - as long as we run distinct phases. It will need some clever script design, but certainly feasible.

Ideally, we'll have at least module-inlined as many routines as possible which will actually remove them as a dependency for a given call site. I guess that has to happen as part of stage 1?

I don't know how reliable inlining is all-in-all ;) I assume we could just run validate to check if this is possible (and if so, don't issue the dependency). Or we could even create the full Fortran and write it out, and then in a later phase 'transmute' (I don't like that term ;) ) these files to add the directives.

From a build-system point of view, I still think my concept gives us all the flexibility we need:

phases:
  # We can define which phases we want to run here. Each phase then is specified in details, i.e. can even be stored
  # in a separate directory etc
  - dsl
  - transmute_after_dsl

dsl:    # details for this phase
  comment: "PSylone DSL Phase"
  api: lfric    # no api --> transmute

  script_dir: ${target}/psykal    # Base dir for all scripts - can be shared, or phase specific

  # The first two directives reproduce the default PSyclone triggering used
  # in LFRic:

...
   Various ways of specifying which script to run for which file.

transmute_after_dsl:
# rinse and repeat

It needs to be extended to allow specification of which files to run on, and keyword arguments, I'll get to this once I have the first steps finished.

The code can even read several of these yaml files and combine them, which means this work can be reused

This will allow us easily switch between 'try automatically as many files as possible, or 'check that it still works for all files based on manual exclusions ' or so.

Also, if I am not mistaken, If we store the information per file in a central file, then we have overall O(n^2), since n files will need to sort out the information for n files when updating its information.

This is imho huge overhead for no gain. ATM, handling this is trivial: you take a dictionary[str, set[str]], use the module name as key, and add all functions to the set.

Individual files is definitely the KISS solution imho.

This makes me think that we are not understanding each other because I would have said the exact opposite :) Writing to a central file is O(n), each file adds its contribution. And the solution is much simpler. You don't have to find them involving the ModuleManager, you don't have to merge their contribution when transmuting, and the file just contains the same dictionary[str, set[str]].

That said, I don't want to completely derail the conversation, the imortant bit is whats the workflow, not so much if the info is stored in a centralised or decentralised file.

Maybe some pseudocode could show that I was thinking:

Phase1:

Module1  <----- Dependent1 : has a loop that calls Module1:subA
           |
           |--- Dependent2 : has a loop that calls Module1:subB

def trans(psyir):
	for each loop to offload:
		for each call:
		    try:
			   ModuleInline().apply(call)
		    except TransformationError as err:
		        if err is that the module call used external symbols:
		        	for each of these symbols + static analysis of its indirect calls
                         globalfile[external_symol.interface.name].append(external_symbol.name)

After phase 1 we have an annotions.yml with:

{
'Module1': ['subA', 'subB']
}

Phase2:

def trans(psyir):
	for container in psyir:
		if container.name in annotations.yml:
		   for subroutine in container:
		       if subroutine in annotations.yml[container.name]:
		       	   OMPDeclareTargetTrans.apply(subroutine)
	for each loop to offload:
		for each call:
		    try:
			   ModuleInline().apply(call, assume_externals_are_valid=True)

Maybe some pseudocode could show that I was thinking:
'''
{
'Module1': ['subA', 'subB']
}
'''

I think I got this (we really need to be in one room with a white board - and I am happy to let this rest till Monday).

My issue is: what if you do a partial build (i.e. you still have the same, previous central file) , because (say) you removed a dependency subA from in a file. Then you don't (locally, while processing this file and only have the central file you mentioned) enough information to know if you can remove subA from Module1 or not, since there is no information in the central file if the Module1: subA in the file was coming only from the file you modified, or from something else as well.

We could add a kind of reference counting (subA was listed 3 times)? But that feels fragile (what if there is a build that aborts, after writing the central file, but before writing the psy-layer? Which can certainly happen, since it's the app transformation that writes the central file, while PSyclone write the psy layer)? You end up with an inconsistent state on disk. With one-file-per file, the incorrect dependency file will be overwritten automatically when you restart the build.

If you create this information from individual files, it will always be up-to-date. Basically, trade off of inodes vs code-maintainability :) Here my read-in code with one dependency file per file (for routines, I haven't touched symbols yet, but it will be the same):

        all_module_info = defaultdict(set)
        for yaml_path in all_files:
            with yaml_path.open("r", encoding="utf8") as stream:
                info = yaml.safe_load(stream)
            for module_name, module_info in info.items():
                all_module_info[module_name].update(module_info["routines"])

I can't judge the performance impact tbh - one process reading a few 100 or 1000 small yaml files (which might be cached on the node that builds), vs using locks to serialise one common file (which might be expensive on lustre), but in order to get this fixed quickly and easy to maintain, I certainly prefer one dep-file per source file.

I think, this is actually just a tiny implementation detail. My suggestion: we start with something simple, and if it should turn out to be too slow (maybe serial reading of the around 2700 small files which we might have in lfric_atm in takes too long??), we can still change this, since both writing the file, and determining the list of files and functions/variables to mark up is in our control (well, I would prefer this, but it could of course be moved into lfric. Us doing that would have the advantage that we can find a solution that will work for both NEMO and LFRic, and can control fully it).

I will put this on hold till the telco ;)