Skip to content

(Towards #2384) directive codeblocks#3014

Merged
arporter merged 25 commits intomasterfrom
2384_directive_codeblocks
Jul 18, 2025
Merged

(Towards #2384) directive codeblocks#3014
arporter merged 25 commits intomasterfrom
2384_directive_codeblocks

Conversation

@LonelyCat124
Copy link
Collaborator

Could be closes, but I'd like to do better than codeblocks if possible (though it'd be a lot more work).

First commits worked for a tiny test case for me with keeping directives, but at this stage I've not added it to the psyclone command yet.

@codecov
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (10602d4) to head (759af38).
Report is 26 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #3014    +/-   ##
========================================
  Coverage   99.92%   99.92%            
========================================
  Files         366      366            
  Lines       51566    51677   +111     
========================================
+ Hits        51526    51637   +111     
  Misses         40       40            

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LonelyCat124
Copy link
Collaborator Author

I now have support and tests for using this with GOCean and LFRic. This required modifying a few places in the code to copy over comments that otherwise didn't, and also enabling parse_fp2 to take an ignore_comments kwargs.

Those changes still need unit testing.

@LonelyCat124
Copy link
Collaborator Author

@sergisiso @arporter This is ready for a first look now.

At the moment I didn't change any of the behaviour for non-directive comments at the end of a code region. We in theory have functionality to turn those into CodeBlocks as well, but since that can affect PSyclone usage I didn't enable that.

If you think its reasonable, I think a Comment class or similar could be used to encapsulate those if required, or an addition to ScopingNode to handle end of section comments.

The other step would be to start converting the directives from codeblocks into actual directives to mix with PSyclone generated ones, but that is very much a "next step" that is a lot more difficult - at least for those directives that expect a schedule etc.

@sergisiso
Copy link
Collaborator

@JulienRemy @schreiberx This PR converts comments to CodeBlocks using the "keep_directive" option that you added to psyclone's frontend. I assume you did this for Poseidon, could you check if this change will cause any issues for you? Are all the directives that you look for in the variable declarations or are they also inside the execution part of the code?

@LonelyCat124
Copy link
Collaborator Author

LonelyCat124 commented Jun 25, 2025

@arporter @sergisiso I'm satisfied with the change for the directives being aware (with limits) of the symbols they contain now - so its ready for review again now.

There can be false positives, however we can't do much about that (e.g. if there's a varaible named private it would catch private as a symbol in an OpenMP clause, but that probably only leads to playing safe.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nearly there now - just a bit of refactoring so that the new utility method can be used in one other location to remove duplication. Please also bring up-to-date with master (which brings back the Python 3.9 testing).

@LonelyCat124
Copy link
Collaborator Author

I missed updating the integration tests too - though I'm not sure how to do this. The LFRic build system is out of PSyclone's control right? Or do you mean the examples? @arporter

@arporter
Copy link
Member

I missed updating the integration tests too - though I'm not sure how to do this. The LFRic build system is out of PSyclone's control right? Or do you mean the examples? @arporter

Oh yes, I forgot that we now just checkout LFRic in its entirety. It will have to just be the LFRic examples. NEMO is slightly easier in that the source is kept on the CI machine. However, if we add these new flags now then we'll break everyone else's PRs so I suggest we do that later as a separate PR.

So, to summarise, if you could just update (some of) the LFRic and NEMO examples to use these new flags that would be great.

@LonelyCat124
Copy link
Collaborator Author

Added now. Assuming testing is ok will send back to you.

@arporter
Copy link
Member

Unfortunately we got a signal 11 when building NEMOv5 with OMP offload:

nvfortran-Fatal-/apps/spack/psyclone-spack-25.3/spack-repo/opt/spack/linux-centos9-skylake_avx512/gcc-14.2.0/nvhpc-24.11-kugacfrugq23e3yjmum24lw2buc67lxd/Linux_x86_64/24.11/compilers/bin/tools/fort2 TERMINATED by signal 11
fcm_internal compile failed (512)
make: *** [/home/me/NEMOv5_Jan25/tests/BENCH_OMP_OFFLOAD_NVHPC/BLD/Makefile:3708: zdfsh2.o] Error 1

Is this a known issue @sergisiso ?

@sergisiso
Copy link
Collaborator

yes, it happens intermittently, sometimes rerunning the action is enough

@arporter
Copy link
Member

Re-triggering the CI run 'fixed' it so I'll work on merging this PR now.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good now.
Will proceed to merge.

@arporter arporter merged commit faef37e into master Jul 18, 2025
11 checks passed
@arporter arporter deleted the 2384_directive_codeblocks branch July 18, 2025 10:47
arporter added a commit that referenced this pull request Jul 18, 2025
sergisiso pushed a commit that referenced this pull request Jul 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants