Repository navigation
Change subfolders for optimisations from site to architecture - #767
James Bruten (james-bruten-mo) merged 14 commits into
Conversation
Matthew Walker (mattatmet)
left a comment
Matthew Walker (mattatmet)
left a comment
There was a problem hiding this comment.
Looks all good to me and makes sense to change. The only changes I would suggest are to update the documentation. There are a few references to sites in the psyclone part of the docs that could do with being adapted to this new structure.
Sure, I've updated the apps docs to reflect the changes in the PR. |
Looks great, thanks! |
|
Benjamin Went (@MetBenjaminWent) can you explain to me the duplication across site macros? Are these intended to be edited such that the default may not be |
| task_values["build_memory"] | ||
| ) }} | ||
|
|
||
| {# Set the psyclone transformation #} |
There was a problem hiding this comment.
As the options for each site are all the same, I think it'd be better to keep that logic here rather than having it in the site macros files
There was a problem hiding this comment.
The idea was that each site might still have their own scripts requirements. It looks like duplication, but it's not.
Currently there isn't a usecase for another site having another option, but it'll come as soon as we start having GPU specific sites and Psyclone scripts.
If we raise it up to the runtime build, sites can't set it induvdually anymore, which is not the aim of this PR (to remove duplication, but leave the door open for the origonal intent of the site folders).
I'm planning like to leave it in the macros for now, unless we can think of a better way of sites setting this flag.
Macros felt like the right place.
I could as a comprimise set the default in the generate runtime to minimum (whuch is the fallback default), and then the macros override?
There was a problem hiding this comment.
What if this is set at the bottom of the task definitions for each site and app?
for example, in rose-stem/site/nci/lfric_atm/tasks_lfric_atm.cylc:
{% if "psyclone_transformation" not in task_dict %}
{% do task_dict.update({"psyclone_transformation": "MY_SITE_DEFAULT"}) %}
{% endif %}
There was a problem hiding this comment.
Otherwise the default is cpu
There was a problem hiding this comment.
I'm not opposed to it being set by each site in their respective cylc files.
But that's also what the marcos are doing?
The default should be minimum, and overidden to cpu
There was a problem hiding this comment.
Also due to the default task definitons, there is always default in task_values["psyclone_transformation"]
There was a problem hiding this comment.
That's fair, if there's a requirement for sites to be able to override then I'm happy with the macros approach.
There was a problem hiding this comment.
Thanks James
Yes, the intent is that each site can still pick a different script method. When GPUs come along, those sites will put It's so that we have preserved the origonal intent of site specific scripts, but removed the duplication. But sites still need a method of picking different scripts, and thats where the macros come in. |
Oakley Brunt (oakleybrunt)
left a comment
Oakley Brunt (oakleybrunt)
left a comment
There was a problem hiding this comment.
Since James is happy with the implementation then I'm happy. All approved, this is a good change.
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Happy with these changes now
4c470df
into
MetOffice:main
|
This change appers to break our routine building of make: Entering directory '/var/tmp/lfric_apps/applications/jedi_lfric_tests'
Makefile:49: *** PSyclone transformation dir "meto-ex1a" does not have a global.py script. Stop.
make: Leaving directory '/var/tmp/lfric_apps/applications/jedi_lfric_tests'Any advice how to fix? |
PR Summary
We have a-lot of duplication of the scripts due to the site name. Originally the intention of the site folder was to contain within it the ideal optimisations, given it's specific hardware architecture requirements.
However, to run the scripts, we are forced to duplicate the scripts for other sites, even though they require no specific site changes.
If we move from sites to hardware architecture, and allow the site to point at specific architecture as required, we maintain the origonal principle, but reduce the duplication and overhead updating all of the scripts together.
To begin with we will have:
With space to expand to allow for example:
(Please use underscore rather than a dash in the names, this causes issues with FAB)
Where these are then set by the sites specific marco, to dictate the type of architecture these will use.
This means CPU sites which all use EXs no longer need to have duplication given the site name, however with scope to allow custom versions of the scripts if the sites require it.
Discussion to get other perspectives: https://github.com/MetOffice/lfric_apps/discussions
Also discussed in a sync with STFC and EPCC, they have no issues with this PR.
Sci/Tech Reviewer: Matthew Walker (@mattatmet)
Code Reviewer: Oakley Brunt (@oakleybrunt)
Umbrella: #424
Issue: #766
Code Quality Checklist
Testing
trac.log
Builds have failed due to usual AZ timeouts - I spent all day re-sending them, and these still failed due to leaving work.
Test Suite Results - lfric_apps - swap_site_to_architecture/run6
Suite Information
Task Information
❌ failed tasks - 5
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