Repository navigation
Add a basic PSyclone test to catch PSyclone issues early for Transmute - #795
James Bruten (james-bruten-mo) merged 5 commits into
Conversation
| end module my_mod''') | ||
| omplooptrans = OMPParallelLoopTrans() | ||
| loop = psyir.walk(Loop)[0] | ||
| omplooptrans.apply(loop, force_private=['array1','array2'], ignore_dependencies_for=['array1','array2']) |
There was a problem hiding this comment.
OMPParallelLoopTrans has the privatise_arrays option which will hopefully do away with the need for the 'force_private` - worth a try.
There was a problem hiding this comment.
(I've just realised this doesn't show up in our docs so I'm talking to LonelyCat124 (@LonelyCat124) about that.)
There was a problem hiding this comment.
After discussing with LonelyCat124 (@LonelyCat124), I realise I was wrong - privatise_arrays isn't going to work with this transformation :-(
There was a problem hiding this comment.
I want to keep this test mirroring what is in the majority of the Transmute scripts currently on trunk as a means to catch when there is a change in the PSyclone repo that affects the scripts, as an early waning system basically. We also don't have any Transmute testing coverage really, so this needs to act as that till we have a better solution.
If we align this test now with the PSyclone repo, this test will not serve it's purpose.
Many thanks for the info tho.
Further thoughts, need to re-review.
Oakley Brunt (oakleybrunt)
left a comment
There was a problem hiding this comment.
How are you integrating this into the working pattern of devs? Is this expected to be part of the GitHub Actions CI/CD or maybe part of the rose-stem test-suite?
So these tests get run when the rose app So this test where I ran the rose app on it's own to make sure it was linked in: And this one where I ran the group |
Oakley Brunt (oakleybrunt)
left a comment
There was a problem hiding this comment.
Looks good, happy to approve
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
This is quite a neat way of running these in the unit tests, so happy to approve
ee900ed
into
MetOffice:main
We have a number of Transmute PSyclone scripts on trunk, however we have little awareness of when they fail.
We log out when Transformations fail in the scripts, however there is alot of false positives, by design of how the scripts currently work where these 'failures' are as part of how the script functions.
Whilst we need to look at this logging (IE what and when we log, and how we filter out genuine issues), we can also implement something simple which would act as an early warning indicator of an issue with the Transmute scripts on trunk.
This can come about with a change of PSyclone versioning, and due to the above issues, difficult to detect. #630 was prompted by such as issue which was not initially detected when it arose.
This PR contains a new simple test which contains a pattern commonly utilised by the Transmute scripts on example source which requires specific things of the Psyclone transformation in order to be successfully parallelised.
As long as this pattern exists in Transmute, this test should remain as it is.
For example: https://github.com/MetOffice/lfric_apps/blob/main/applications/lfric_atm/optimisation/meto-ex1a/transmute/global.py
Note I've suppressed the PSyclone warnings that we have pulled through as to not clog up the python testing output.
It still fails as expected if the source changes.
PR Summary
Sci/Tech Reviewer: Oakley Brunt (@oakleybrunt)
Code Reviewer: James Bruten (@james-bruten-mo)
Issue: #755
Umbrella: #424
Code Quality Checklist
Testing
trac.log
Failures are due to issues with AZ and PSyclone, unrelated to these changes.
Test Suite Results - lfric_apps - psyclone-test/run4
Suite Information
Task Information
❌ failed tasks - 3
⌛ waiting tasks - 10
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review