Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add staged CABLE driver for CABLE configurations #461

Merged
merged 20 commits into from
Sep 25, 2024
Merged

Conversation

Whyborn
Copy link
Contributor

@Whyborn Whyborn commented Jul 11, 2024

This driver is intended to facilitate spin-up CABLE configurations, which involve usually many stages with differing science configurations. An example experiment exists here, with some documentation: Staged_CABLE

@pep8speaks
Copy link

pep8speaks commented Jul 11, 2024

Hello @Whyborn! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-09-24 12:38:22 UTC

@Whyborn Whyborn changed the title DRAFT: Add CABLE-POP driver for advanced configurations Add CABLE-POP driver for advanced configurations Jul 29, 2024
@Whyborn Whyborn changed the title Add CABLE-POP driver for advanced configurations Add staged CABLE driver for advanced configurations Jul 29, 2024
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

I have some questions and suggestions.

payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
@Whyborn
Copy link
Contributor Author

Whyborn commented Aug 6, 2024

I'll add some tests before requesting another review.

@aidanheerdegen
Copy link
Collaborator

Are you ready for another review @Whyborn?

@Whyborn
Copy link
Contributor Author

Whyborn commented Sep 9, 2024

I had requested another one, but I have been doing some thinking since the Payu workshop session to improve the handling of restarts. I'll cancel the current request and add a new one after I make (or not make) some changes.

The files retrieved by get_prior_restart_files are automatically prepended by the experiment restart directory, which is the most recent restart. Adding '..' to redirect to the older restart directories is also not an option, as the generated links will inherit the same redirection. Also handed situation where user only provides a stage namelist, with no corresponding master.
@Whyborn
Copy link
Contributor Author

Whyborn commented Sep 23, 2024

I've made some adjustments to the way we retrieve the restarts, see the unresolved conversation. I've added tests to capture most of the desired functionality, but I was struggling to build a good test for retrieving restarts.

@Whyborn Whyborn requested a review from ccarouge September 23, 2024 06:09
Copy link
Contributor

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

I haven't looked at the tests and I'm running out of time so I'll send what I've picked up.

I would refactor _prepare_stage(). I don't like that it calls _apply_stage_namelists(). I would transform _prepare_stage() into a _get_stage_name() function that returns the current stage name. Then call that from _apply_stage_namelists() or from within setup().
Then I would introduce a deal_with_configuration_log() function (name to be determine) that is called at the end of setup() and deals with saving the configuration log file (call the function you have for that) and copying it to the work directory.

One final comment for @aidanheerdegen , do you have any aspiration to use Pathlib instead of os.path in payu? If so, maybe it's a bad idea to introduce a new driver based on os.path. Your call @aidanheerdegen .

payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
@aidanheerdegen
Copy link
Collaborator

I think I'm at the end of my utility as a reviewer. I see @ccarouge has a few more suggestions, but after that probably best to merge it and start using it and iterate if there are problems, or improvements required. You're not touching any other drivers, so it's fairly side-effect free and you can test it, as can others.

Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

I've approved as Claire is away in case you're ready to merge @Whyborn

payu/models/staged_cable.py Outdated Show resolved Hide resolved
payu/models/staged_cable.py Outdated Show resolved Hide resolved
@Whyborn Whyborn changed the title Add staged CABLE driver for advanced configurations Add staged CABLE driver for CABLE configurations Sep 25, 2024
@Whyborn Whyborn merged commit f4af1ff into master Sep 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants