Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughThe shutdown logic in executorlib/task_scheduler/interactive/spawner_flux.py now wraps the final future.result() call in a contextlib.suppress for flux.job.event.JobException after cancellation, changing error handling to ignore that exception. Imports updated to include contextlib. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TS as TaskScheduler
participant FF as Flux Future
TS->>FF: cancel()
Note right of FF: Future is cancelled
TS->>FF: wait/poll until done
alt result retrieval
TS->>FF: result()
FF-->>TS: JobException
Note over TS: Exception suppressed (no propagation)
else no exception
FF-->>TS: result value
end
TS-->>TS: proceed with shutdown
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
executorlib/task_scheduler/interactive/spawner_flux.py (1)
151-152: Restrict or log suppressed JobException
Suppressing allflux.job.event.JobExceptioninspawner_flux.py(lines 151–152) hides legitimate failures. Either guard onself._future.cancelled()before callingresult(), or catch and log the exception (e.g., at debug level) to surface unexpected errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executorlib/task_scheduler/interactive/spawner_flux.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
executorlib/task_scheduler/interactive/spawner_flux.py (1)
executorlib/task_scheduler/file/shared.py (1)
result(24-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: notebooks_integration
- GitHub Check: unittest_openmpi (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_slurm_mpich
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_mpich (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_mpich (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_flux_mpich
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_old
- GitHub Check: unittest_win
🔇 Additional comments (1)
executorlib/task_scheduler/interactive/spawner_flux.py (1)
1-1: LGTM!The import is necessary for the
contextlib.suppress()usage in the shutdown method.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 34 34
Lines 1686 1688 +2
=======================================
+ Hits 1655 1657 +2
Misses 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Summary by CodeRabbit