371 fab transmute support - #442
Joerg Henrichs (hiker) wants to merge 135 commits into
Conversation
…nfiguration to compile skeleton.
… build system and scripts.
Matthew Hambley (MatthewHambley)
left a comment
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
I don't think the big lists of work is the way we want to go in the long run, but given this change is written, and modifying it would likely take a considerable amount of effort, we can go ahead with this now and change it later.
| # A simplified example to use mirrors could be (which would | ||
| # typically be implemented in a derived, site-specific class) | ||
| # root = Path("/root/of/mirrors") | ||
| # mirrors = {"git@github.com:MetOffice/casim.git": root / "casim", | ||
| # "git@github.com:MetOffice/jules.git": root / "jules", | ||
| # } | ||
| # for dependency in dep_info.get_repo_names(): | ||
| # repo_infos = dep_info.get_repo_info(dependency) | ||
| # for source_ref in repo_infos: | ||
| # if source_ref.source in mirrors: | ||
| # logger.info(f"Using mirror " | ||
| # f"'{mirrors[source_ref.source]}' for " | ||
| # f"'{source_ref.source}") | ||
| # source_ref.source = mirrors[source_ref.source] |
There was a problem hiding this comment.
Can this example be included in the docstring, and therefore the documentation?
There was a problem hiding this comment.
Depends on previous solution. Imho, moving DependencyInfo to Fab would be best (avoiding code duplication between lfric and UM and potentially other codes; and the documentation would nicely go into the Fab documentation of 'usage pattern'
|
|
||
| :param build_config: the Fab build configuration instance | ||
| ''' | ||
| print("SiteConfig default GNU") |
There was a problem hiding this comment.
Is this left over debug or intentional logging. If the latter, is there a way to use the logging framework?
There was a problem hiding this comment.
Oops, indeed a debug print. Removed
| # initialising compilers | ||
| self._args = args | ||
|
|
||
| def update_repos(self, dep_info): |
There was a problem hiding this comment.
Type hinting please.
There was a problem hiding this comment.
Now this is a really difficult question.
IIRC, we decided to have DependencyInfo added to LFRic apps only. So, I can't typehint it here, the class is not known.
If I would move this function into lfric_apps, we would in one way or another duplicate a lot of code:
- We add site_config settings to apps. Then we have two sources of 'standard lfric' compilation flags, one in core, one in apps.
- We add this as an application specific setting (which will be added for lfric_atm in https://github.com/hiker/lfric_apps/tree/513_more_fab_scripts). Then we still have the problem if/when there are other applications that needs a git checkout (not certain if there are tbh), that we need to duplicate the mirror handling.
- We add site_config settings to lfric_apps, and inherit from the site_config settings in core. That might work, though it complicates the setup a bit more.
- We add DependencyInfo to Fab instead (see Supporting dependencies.yaml and mirrors? fab#563 where I suggested this). Note that the UM also uses dependency info, so moving this class into Fab would also solve this code duplication.
Amy comments?
There was a problem hiding this comment.
DependencyInfo is coming into Fab, so I've updated this to now correctly typhing to Fab's DependencyInfo (not yet merged in, so we need all Fab PRs to be merged first)
| # Test case 3: No PSykal but optimisation directory | ||
| optimisation_folder_path = (tmp_path / "optimisation" / "default-default" / | ||
| "psykal") | ||
| # Test case 2: No PSykal but optimisation directory |
There was a problem hiding this comment.
Can these various test cases be split into separate test functions? Maybe with fixtures for commonly used harnes? Maybe parameterised tests?
There was a problem hiding this comment.
I split a set off into different methods of a base class. I tried to include more, but it was quite difficult (since some tests needed to mock Fab base methods, some Fab functions).
…re into 371_fab_transmute_support
…ding Fab implementation).
github-actions
Bot
added
the
cla-modified
github-actions
Bot
removed
the
cla-modified
|
Matthew Hambley (@MatthewHambley) , this is basically ready, but needs Fab PR MetOffice/fab#600 to be merged (and ideally a Fab release) |
|
Note that while this PR has basically been reviewed (pending on Fab work), recent work on lfric_core/apps required a small change: there is now a command line option that will allow anyone to select the (upcoming :) ) optimisation directories (like The implementation uses Python string templates, so that a psyclone_control file can select to specify the optimisation directory flexible: or hardcode: This will already allow sites to switch to the new directory structure (while the old build system relies on links to work). And the flexibility also makes it easier to e.g. write GPU-specific PSyclone control files |
5 participants
PR Summary
This PR adds support for transmutation. While this is atm not used in lfric_core, it could be useful to have it available here (since it allows e.g. testing of PSyclone transmute steps with the much smaller skeleton apps, which gives a much quicker turnaround compared to compiling lfric_atm).
Sci/Tech Reviewer:
Code Reviewer: Matthew Hambley (@MatthewHambley)
This adds the psyclone control yaml file, which can be used to specify in detail when/how to run dsl and transmute phases on code.
Code Quality Checklist
Testing
I have run the simple example for skeleton, but not the rose-stem suite. I have also similarly confirmed that lfric_atm works with this change (once the lfric_atm Fab script is added). Again only using the simple example build and test, not any cylc suite.
trac.log
Security Considerations
Performance Impact
AI Assistance and Attribution
The psyclone_control_test.py file has been initially created with copilot, but was then manually cleaned up and fixed.
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review