chore: remove redundant rm.ExternalPreemptionPending interface #10071
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for determined-ui canceled.
|
stoksc
commented
Oct 16, 2024
| err := task.DefaultService.Signal( | ||
| model.AllocationID(req.AllocationId), | ||
| task.TerminateAllocation, | ||
| "preempted by the scheduler", |
There was a problem hiding this comment.
kept the same message but it's not a particularly great message
|
hilarious i got "code review required" by both backend and cluster mgmt. may need to tweak though filters some. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10071 +/- ##
==========================================
+ Coverage 54.42% 54.43% +0.01%
==========================================
Files 1262 1262
Lines 158901 158886 -15
Branches 3631 3632 +1
==========================================
+ Hits 86474 86487 +13
+ Misses 72293 72265 -28
Partials 134 134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
rb-determined-ai
approved these changes
Oct 17, 2024
maxrussell
approved these changes
Oct 17, 2024
rb-determined-ai
added a commit
that referenced
this pull request
Oct 22, 2024
The only test in this series was an e2e test sure that we caught slurm's SIGTERM, reported that to the master via an API call, and that training could would then see should_preempt() return True. This didn't need to be an e2e test; it can be a unit test to make sure our SIGTERM handler is working, plus rerouting the API logic to pass through the our normal preemption handling (#10071). There is already plenty of coverage ensuring should_preempt() works in python code. So by adding that unit test here and following the refactor of #10071, it is safe to remove the e2e_slurm_preemption series entirely. This is part of a larger effort to get rid of our znode tests, which are notoriously unreliable.
rb-determined-ai
added a commit
that referenced
this pull request
Oct 22, 2024
The only test in this series was an e2e test sure that we caught slurm's SIGTERM, reported that to the master via an API call, and that training could would then see should_preempt() return True. This didn't need to be an e2e test; it can be a unit test to make sure our SIGTERM handler is working, plus rerouting the API logic to pass through the our normal preemption handling (#10071). There is already plenty of coverage ensuring should_preempt() works in python code. So by adding that unit test here and following the refactor of #10071, it is safe to remove the e2e_slurm_preemption series entirely. This is part of a larger effort to get rid of our znode tests, which are notoriously unreliable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
Description
The
ExternalPreemptionPendingmethod always essentially calledallocation.Signal, but through 2/3 pointless other calls/message passes. This just cuts out a few middlemen.I did it now because it stood in the way of a test @rb-determined-ai was trying to remove.
Test Plan
This is code removal, needs no testing. It even swaps to tested code instead!
Checklist
docs/release-notes/See Release Note for details.