371 fab transmute support - #442
Conversation
…nfiguration to compile skeleton.
… build system and scripts.
…be required for lfric_apps. Minor code updates.
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?
| # 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?
…re into 371_fab_transmute_support
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