Implement add_rose_experiment and delete/archive/run_experiments to CylcRoseManager#59
Implement add_rose_experiment and delete/archive/run_experiments to CylcRoseManager#59edoyango wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 948 1023 +75
=========================================
+ Hits 948 1023 +75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if not experiment_path.is_dir(): | ||
| raise ValueError(f"Experiment path '{experiment_path}' does not exist or is not a directory.") | ||
|
|
||
| run_path = Path("/scratch") / project / getpass.getuser() / "cylc-run" / rose |
There was a problem hiding this comment.
The logic to build the run path seems to be very Gadi specific. I would prefer to keep this as general as possible, so could the function take the run_path as argument instead of the project?
There was a problem hiding this comment.
looks like i can get cylc's expected run directory with cylc get-global-config -i '[hosts][localhost]run directory'. The downside being that you need cylc7 module loaded. Would that be preferred, or explicitly pass in run_path?
There was a problem hiding this comment.
I would keep this simple and explicitly pass the run path. Users can always use the cylc command in their notebooks if they feel like it.
| if all_experiments and experiments is not None: | ||
| raise ValueError("Pass either experiments=[...] or all_experiments=True") | ||
| if not all_experiments and not experiments: | ||
| raise ValueError("No experiments specified. Pass either experiments=[...] or all_experiments=True") | ||
|
|
||
| existing = set(self.experiments.keys()) | ||
| names_to_delete = existing if all_experiments else set(experiments) | ||
|
|
||
| unmanaged = [e for e in names_to_delete if e not in existing] | ||
| if unmanaged: | ||
| raise KeyError( | ||
| f"Experiments {unmanaged} are not managed by this CylcRoseManager " | ||
| f"(existing: {existing}). Please check the names and try again." | ||
| ) |
There was a problem hiding this comment.
Looks like these lines are identical to the payu ones. Maybe these could be moved to the base class?
There was a problem hiding this comment.
Thanks for the changes!
If I may, I would suggest a slight different approach, more OOP.
Instead of creating a helper function in the base class, I would add a delete_experiments method in the base class that contains the shared code and then calls a deferred method that deletes a single experiment. The deferred method would then be implemented in each of the concrete managers. That way the dependency is inverted and the control flow points from the abstract class to the children.
|
@edoyango Forgot to say in the review that you need to rebase on main to get the Conda build CI failure fixed. |
…to CylcRoseManager Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
edoyango
force-pushed
the
cycl-rose-manager
branch
from
fec1a39 to
4f4890a
Compare
Both PayuManager and CylcRoseManager had identical logic for validating and resolving experiment name selections. Move it to a single base-class helper so each subclass only owns its deletion mechanics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
This PR implements features in CylcRoseManager that are missing (compared to PayuManager).
run_experiments uses the
rose suite-runcmd as I don't think there's a good python api to use for rose.rose suite-runis executed with--no-gcontrolso the gui isn't launched. Like inPayuManager, the experiments' status is set to DONE immediately after launching the pipeline.