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

LocalProcess class #206

Draft
wants to merge 15 commits into
base: scheduler_class
Choose a base branch
from
Draft

Conversation

dafyddstephenson
Copy link
Contributor

@dafyddstephenson dafyddstephenson commented Dec 23, 2024

This PR is chained off of #205 (#175) and creates a base class ExecutionHandler of which both SchedulerJob and LocalProcess (a newly defined class) are subclasses. This allows consistent behaviour whether or not the user is running on a scheduler-managed system: an object is returned by Case.run() that can be used to track the state of the run, even if that run is happening in the background locally.

⚠️ NOTE: The base of this PR is currently set to the scheduler_class branch. Please first merge #205 and update the base to main before changing the status of this PR from draft. ⚠️

Dafydd Stephenson added 2 commits December 23, 2024 09:42
- Define ExecutionHandler class, set SchedulerJob as a subclass
- Define LocalProcess subclass of ExecutionHandler
@dafyddstephenson dafyddstephenson added the internals non-user-facing changes label Dec 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2024

Codecov Report

Attention: Patch coverage is 96.68508% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.78%. Comparing base (431fb69) to head (18383ee).

Files with missing lines Patch % Lines
cstar/roms/component.py 71.42% 4 Missing ⚠️
cstar/execution/scheduler_job.py 85.00% 3 Missing ⚠️
cstar/execution/handler.py 95.83% 2 Missing ⚠️
cstar/base/component.py 50.00% 1 Missing ⚠️
cstar/case.py 0.00% 1 Missing ⚠️
cstar/marbl/component.py 50.00% 1 Missing ⚠️
@@                 Coverage Diff                 @@
##           scheduler_class     #206      +/-   ##
===================================================
+ Coverage            92.18%   92.78%   +0.59%     
===================================================
  Files                   40       44       +4     
  Lines                 3623     3813     +190     
===================================================
+ Hits                  3340     3538     +198     
+ Misses                 283      275       -8     
Files with missing lines Coverage Δ
cstar/execution/local_process.py 100.00% <100.00%> (ø)
...ts/integration_tests/test_cstar_test_blueprints.py 100.00% <100.00%> (ø)
cstar/tests/unit_tests/execution/test_handler.py 100.00% <100.00%> (ø)
...r/tests/unit_tests/execution/test_local_process.py 100.00% <100.00%> (ø)
cstar/tests/unit_tests/execution/test_pbs_job.py 100.00% <100.00%> (ø)
...r/tests/unit_tests/execution/test_scheduler_job.py 97.18% <100.00%> (ø)
cstar/tests/unit_tests/execution/test_slurm_job.py 100.00% <100.00%> (ø)
cstar/base/component.py 63.38% <50.00%> (ø)
cstar/case.py 55.50% <0.00%> (ø)
cstar/marbl/component.py 82.92% <50.00%> (ø)
... and 3 more

Dafydd Stephenson added 10 commits December 23, 2024 14:05
…ionHandler instance

- Update ROMSComponent.run() logic to create, start, and return a LocalProcess instance on a non-scheduler machine
- Move 'updates' method definition from SchedulerJob into ExecutionHandler base class
- Temporarily comment out ROMS output parsing in ROMSComponent.run()
…l set in 'run()') to prevent premature call to 'post_run()'
- Make ExecutionHandler.updates terminate when status changes from RUNNING
- Add a 'confirm_indefinite' parameter to updates when seconds==0 to skip y/n confirmation if False
- Add / update unit tests
- Update integration tests to print updates
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internals non-user-facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants