502 ignore mo dependencies

[hiker]

Fixed #502 (and fixed a #todo)

[hiker]

First requesting a review from Jason, since he analysed the original failure.

[jasonjunweilyu]

I think this does not solve the problem of analyse being coupled with psyclone, i.e. psyclone and analyse steps use the same fortran analyser:

fab/source/fab/steps/psyclone.py

Line 248 in c0f543a

# todo: We'd like to separate that from the general fortran analyser at some point, to reduce coupling.

With these changes, if psyclone step is called with psyclone(self.config, kernel_roots=[(self.config.build_output)], , i.e. not limiting the kernel_root to the kernel folder, the ignore_dependencies would not take effects in the anaylse step for ignoring Fortran module names in USE statements and 'DEPENDS ON' modules. This is because the fortran analyser is already run in the psyclone step without having the ignore_dependencies as input:

fab/source/fab/steps/psyclone.py

Line 250 in c0f543a

fortran_analyser = FortranAnalyser(config=config)

Once a file is analysed in the psyclone step, it would not be analysed again in the analysed step. The 'DEPENDS ON' files would indeed be ignored with the changes proposed in mo.py.

Maybe adding a way to trigger re-analysis of a file if the fortran analyser inputs have changed could be a solution?

[hiker]

I think this does not solve the problem of analyse being coupled with psyclone, i.e. psyclone and analyse steps use the same fortran analyser:

fab/source/fab/steps/psyclone.py

Line 248 in c0f543a

# todo: We'd like to separate that from the general fortran analyser at some point, to reduce coupling.

With these changes, if psyclone step is called with psyclone(self.config, kernel_roots=[(self.config.build_output)], , i.e. not limiting the kernel_root to the kernel folder, the ignore_dependencies would not take effects in the anaylse step for ignoring Fortran module names in USE statements and 'DEPENDS ON' modules. This is because the fortran analyser is already run in the psyclone step without having the ignore_dependencies as input:

Maybe my patch worked, because it also avoided the KeyError?

Or is the issue when the mo conversion (converting from dependencies in comments to file dependencies) is running? I think that phase is only running at the end of the analysis step (and not during the PSyclone step). That means that indeed the dependency to (say) c_shumlib_byteswap is added during the PSyclone analysis, but then ignored when the conversion is done during the analysis step?

That might be acceptable.

Checking my runs, I just noticed one additional change: I changed the ignore dependency in lfricinputs from c_shum_byteswap.o to c_shum_byteswap.c. Doing a clean build: indeed, when the dependency is specified as .c (as it is in git atm), this is not found and then triggers the new error message (and which would crash fab without this change).

But, when I use the .c name, then this is properly ignored. So, without checking the code, I would assume that fab converts the c_shum_byteswap.o in the DEPENDS to .c, which it then cannot find. OK, I now checked, indeed fortran.py does:

                analysed_file.mo_commented_file_deps.add(
                    dep.replace(".o", ".c"))

So that might be the real, or an additional problem: the conversion of .o to .c is done too early (i.e. before converting the DEPENDS dependencies to normal file dependencies). We should store the original .ofile, use this for ignore tests, but then add the .c file as file dependency (if not ignored).

Searching during the PSyclone analysis step only in .../kernels means that the files with DEPENDS are not analysed during PSyclone (where no ignores are set). And, if we search the parent, these files get analysed and the dependencies are not ignored, but the actual crash happens later during the conversion from DEPENDS dependencies to file dependencies.

So, a few things to evaluate:

1. Try to store the dependencies in the original format (.o), not .c. It would certainly be better if the application script specifies the name of modules to ignore exactly the way it is done in the file (i.e. with .o), and the application does not have to convert this to .c. Then my patch works (without the error message about the missing dependencies), and allows the application to specify exactly what is in the source file (.o and not .c).
2. Alternative, maybe just ignore file extension entirely during all ignore tests?
3. We could provide the ignore list to both steps (psyclone and analyse) in the FabBase class. The script would just use a method to add the dependencies to ignore, and this list would then be provided whenever required by the fab base class (instead of the way it is done atm that you overwrite the analysis step, which allows you to add the ignore modules).

[hiker]

I have done that properly now: the analysis step will store the original .o name (if the file is analysed during the analysis step outside of PSyclone, files to be ignored will already be ignored).

When converting the module dependencies to file name (in mo.py), it will now now check for the original (.o) name in the ignore list, then rename the name to have .c. It will then again check if the .c name is to be ignored (just as convenience for the user). And if not, the .c name will be used to look up the required file name.

So for now, in PSyclone's analysis part the ignore_dependencies are not used (so we do get names to be ignored in the analysis data structure). But when the DEPENDEND names are converted to filename, the ones to be ignored will be ignored, and then the analysis data structure will only have filenames that are not supposed to be ignored.

So fixing the original crash is a kind of two step process: first letting the original .o name into the analysis data structure, but then not converting files to be ignored during the analysis step.

Am happy to discuss alternative solutions (e.g. add ignore list to psyclone step?).

But this solution now seems to work properly with LFRic applications (lfricinputs esp), that have a dependency to shumlib and use DEPENDS ON, and with shumlib now being linked as a library (as opposed to previously where shumlib was actually available, so the files would be found)

[hiker]

I have opened #505 to address some incorrect timer issues that I noticed.
But thinking about the issue, it would make sense to add the dependency_ignore information to the PSyclone step as optional parameter.

[hiker]

This now uses the ignore list also while parsing kernels during the PSyclone step. I still left the additional code in mo.py in place, since it's up to the application to make sure both steps receive (the same :) ) list. This way, we get what I think is a better user experience(?), but am happy to discuss this with the reviewer.

[hiker]

I have discussed this with @jasonjunweilyu , and we agreed to add ignoring to the psyclone step (which should avoid some warning messages).

This is now ready for review, @allynt

[jasonjunweilyu]

Thanks @hiker for the efforts. With ignore_dependencies added to the psyclone step and .c file also accepted as input for ignore_dependencies, now the functionality of ignoring certain modules or files is more robust and should satisfy users' needs. Therefore I approve this change.