Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

#1286 Re submit #1358

Closed
wants to merge 6 commits into from
Closed

#1286 Re submit #1358

wants to merge 6 commits into from

Conversation

suiguoxin
Copy link
Member

  • only failed jobs can be re-submitted
  • no trial_id returned when re-submitted successfully
  • else raise err

@suiguoxin suiguoxin requested review from chicm-ms and lvybriage July 24, 2019 03:43
@@ -141,6 +141,7 @@ class NNIDataStore implements DataStore {

public async getTrialJob(trialJobId: string): Promise<TrialJobInfo> {
const trialJobs: TrialJobInfo[] = await this.queryTrialJobs(undefined, trialJobId);
assert(trialJobs.length <= 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if trialJobs.length == 0, then asseration is right but will crash when take value "trialJobs[0]" ?

Copy link
Member Author

@suiguoxin suiguoxin Jul 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is to avoid confict with this PR #1288. I don't know exactly why this assert is added @chicm-ms

@@ -168,6 +169,9 @@ def handle_initialize(self, data):
def handle_request_trial_jobs(self, data):
raise NotImplementedError('handle_request_trial_jobs not implemented')

def handle_resubmit_trial_job(self, data):
raise NotImplementedError('handle_resubmit_trial_job not implemented')

def handle_update_search_space(self, data):
raise NotImplementedError('handle_update_search_space not implemented')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irrelevant to this PR. The indentation is mistyped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why irrelevant ? Which indentation ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -103,6 +103,7 @@ abstract class Manager {
public abstract exportData(): Promise<string>;

public abstract addCustomizedTrialJob(hyperParams: string): Promise<void>;
public abstract resubmitTrialJob(hyperParams: string): Promise<void>;
public abstract cancelTrialJobByUser(trialJobId: string): Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parameter should be trial job id as we discussed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -135,6 +135,42 @@ class NNIManager implements Manager {
return this.dataStore.storeTrialJobEvent('ADD_CUSTOMIZED', '', hyperParams);
}

public async resubmitTrialJob(data: string): Promise<void> {
// get trail job info by jobId
const trialJobId = JSON.parse(data).job_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter should be trial job id, it should not need to be parsed inside this function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

return Promise.reject(
new Error('only failed trials can be resubmitted')
);
}
Copy link
Contributor

@chicm-ms chicm-ms Jul 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This status checking can be moved up to before retrieving hyperParameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@QuanluZhang QuanluZhang closed this Sep 6, 2019
@suiguoxin suiguoxin deleted the re-submit branch November 13, 2019 06:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants