This repository has been archived by the owner on Sep 18, 2024. It is now read-only.
Fix a race condition bug that does not store Trial Job cancel status correctly #707
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
Contributor
yds05
commented
Feb 1, 2019
yds05
commented
- For PAITrainingService, we should rely on the status returned from PAI rest server to change trial job's status, rather than setting it's cancelling status in cancelTrialJob() method, because jobInfoCollector will then reset job's status based on PAI rest server's result.
- For RemoteMachineTrainingService, it almost the same, we should not set trial's status in cancelTrialJob() method.
SparkSnail
approved these changes
Feb 2, 2019
QuanluZhang
reviewed
Feb 8, 2019
| deferred.resolve(); | ||
| } | ||
| }); | ||
|
|
||
| // Set trialjobDetail's early stopped field, to mark the job's cancellation source | ||
| trialJobDetail.isEarlyStopped = isEarlyStopped; |
There was a problem hiding this comment.
I think this line has to be put at the beginning of this function.
There was a problem hiding this comment.
Not very sure why we should put this line to the beginning.
The beginning of this function has many condition checking. If we put this line to the beginning of this function but condition checking failed, the trial job status may be wrong. And also, I didn't see any race condition risk to put it after we issues PAI job stop request.
There was a problem hiding this comment.
agree, then we should put this line before line 328 request(...
QuanluZhang
reviewed
Feb 8, 2019
| trialJob.status = getJobCancelStatus(isEarlyStopped); | ||
| //TODO: delete and move set USER_CANCELLED/EARLY_STOP in getTrialJob | ||
| // Mark the toEarlyStop tag here | ||
| trialJob.isEarlyStopped = isEarlyStopped; |
There was a problem hiding this comment.
this line should be put before the await SSHClientUtility (i.e., line 282)
chicm-ms
approved these changes
Feb 14, 2019
QuanluZhang
approved these changes
Feb 25, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.