(Towards #2971) Moved OMPParallelTrans to psyir.transformations #3280
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## master #3280 +/- ##
==========================================
- Coverage 99.95% 99.93% -0.03%
==========================================
Files 376 377 +1
Lines 53545 53545
==========================================
- Hits 53523 53511 -12
- Misses 22 34 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sergisiso @arporter Mostly a small PR ready for a look, just moves the transformation and fixes the scripts I found. I'm going to set integration tests running now as well in case I missed anything. |
LonelyCat124
temporarily deployed
to
integration
GitHub Actions
Inactive
LonelyCat124
temporarily deployed
to
integration
GitHub Actions
Inactive
LonelyCat124
temporarily deployed
to
integration
GitHub Actions
Inactive
|
integration passes so ready for review. |
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
Thanks Aidan.
Mostly good but I'm worried that we're no longer testing the 'transmutation' part of LFRic-atm - see inline. (I may be mistaken though.)
| # Build the 'meto-ex1a' platform, that includes psyclone-transmutation | ||
| export BUILD_START="${SECONDS}" | ||
| ./build/local_build.py -a lfric_atm -p meto-ex1a -j 16 -w workdir-lfric-atm-transmuted | ||
| ./build/local_build.py -a lfric_atm -p psyclone-test -j 16 -w workdir-lfric-atm-transmuted |
There was a problem hiding this comment.
Does this mean we're no longer testing the 'transmutation' if we're using our own "everything_everywhere..." script? That doesn't seem right?
There was a problem hiding this comment.
Could we patch the transformation script(s) used by LFRic?
There was a problem hiding this comment.
I'm a bit clueless here I just did what @sergisiso suggested I thought - if I misunderstood I can fix that, let me know if so Sergi.
Its probably patchable but I'd need to find the script, its just removing OMPParallelTrans from the transformations import and swapping it to psyir.transformation.
There was a problem hiding this comment.
Does this mean we're no longer testing the 'transmutation' if we're using our own "everything_everywhere..." script?
Yes, but not because we use this file for psykal transformations, but because we created a new optimisation target "psyclone-test" without a transmute folder.
A quick solution would be to also copy the transmute files from meto-ex1a cp applications/lfric_atm/optimisation/meto-ex1a/transmute/ applications/lfric_atm/optimisation/psyclone-test/transmute/ however if any of those also gets OMPParallelTrans from TransInfo it will also fail.
There was a problem hiding this comment.
Oh yeah I'd forgotten this change means OMPParallelTrans is no longer available from TransInfo.
There was a problem hiding this comment.
One place its defined in lfric that is a problem for lfric_atm is unfortunately in lfric_core/infrastructure/build/psyclone/psyclone_tools.py where they use:
from psyclone.transformations import (
Dynamo0p3ColourTrans,
Dynamo0p3OMPLoopTrans,
Dynamo0p3RedundantComputationTrans,
OMPParallelTrans,
)
This might be possible to fix but I'm not sure how, we'd need to patch it somehow - @arporter I think up to you if you want me to try.
Its also imported in applications/lfric_atm/optimisation/meto-ex1a/psykal/algorithm/casim_alg_mod.py, for lfric atm. It is used in a few other application scripts but I assume we don't hit those in our tests.
There was a problem hiding this comment.
This might be possible to fix but I'm not sure how, we'd need to patch it somehow - @arporter I think up to you if you want me to try.
Yes please, I think it's important we keep this. We do already patch lfric for the nvidia compiler:
PSyclone/.github/workflows/lfric_test.yml
Line 101 in 2823f9d
so hopefully it's doable for this too. Or have a little bash script that
finds all .py files, checks for OMPParallelTrans and updates as necessary?
There was a problem hiding this comment.
It could get complicated with patches (I am also breaking lfric_atm scripts in #3275). But since I am updating to git in another PR and lfric_apps is public and on git what do you think if we create a fork of MO:lfric_apps with we do the scripts breaking changes there. Then with the release we can push the changes to the MO repository or at least point it to them in order to do the update.
There was a problem hiding this comment.
That's a good idea Sergi - sounds like the most straightforward way to go.
There was a problem hiding this comment.
@sergisiso Should we do this for lfric_core too? There are some scripts in there that contain base functionality used by lfric_apps (e.g. lfric_core/infrastructure/build/psyclone/psyclone_tools.py)
| # J. Dendy, Met Office | ||
| '''This module provides the OMPParallelTrans transformation.''' | ||
|
|
||
| from psyclone.psyir.transformations.parallel_region_trans import ( |
There was a problem hiding this comment.
If possible (i.e. you don't get circular imports), pls put these import statements in alpha-order by module name.
|
lfric core: 4eec06e08e19ac23a894fcfdad691132ffc68293 |
No description provided.