support display trial log on local mode #2718
Conversation
|
Approve for webui changes. |
JunweiSUN
changed the title
QuanluZhang
requested review from
liuzhe-lz,
chicm-ms,
ultmaster,
QuanluZhang and
SparkSnail
| @@ -325,6 +325,10 @@ class NNIManager implements Manager { | |||
| // FIXME: unit test | |||
| } | |||
|
|
|||
| public async getTrialLog(trialJobId: string, logType: LogType): Promise<string> { | |||
| return this.trainingService.getTrialLog(trialJobId, logType); | |||
There was a problem hiding this comment.
it might be better to catch MethodNotImplementedError here, then return log message like XXX training service does not support retrieving log.
There was a problem hiding this comment.
could you explain how did you fix this one?
There was a problem hiding this comment.
In the latest commit, I revert this to the original version (i.e., not handle the error here) since the front end will check the training service now. The error here could also be an file operation error, so I let the restHander to catch the error throwed here.
| } else { | ||
| throw new Error('unexpected log type'); | ||
| } | ||
| return fs.promises.readFile(logPath, 'utf8'); |
There was a problem hiding this comment.
does this readFile() function need to catch exception?
There was a problem hiding this comment.
This exception should be catched by getTrialLog in restHandler.ts
| @@ -8,6 +8,8 @@ | |||
| */ | |||
| type TrialJobStatus = 'UNKNOWN' | 'WAITING' | 'RUNNING' | 'SUCCEEDED' | 'FAILED' | 'USER_CANCELED' | 'SYS_CANCELED' | 'EARLY_STOPPED'; | |||
|
|
|||
| type LogType = 'TRIAL_LOG' | 'TRIAL_STDERR'; | |||
There was a problem hiding this comment.
Does TRIAL_LOG means stdout log?
There was a problem hiding this comment.
Yes, corresponding to trial.log.
There was a problem hiding this comment.
I think we discussed and decided that it should be ERROR_MESSAGE
There was a problem hiding this comment.
change to TRIAL_LOG and TRIAL_ERROR
| localTrainingService.run() | ||
| await delay(5000); | ||
| //chai.expect(jobDetail.status).to.be.equals('RUNNING'); | ||
| chai.expect(await localTrainingService.getTrialLog(jobDetail.id, 'TRIAL_LOG')).to.be.a('string'); |
There was a problem hiding this comment.
why not compare with 'This is trial log'?
There was a problem hiding this comment.
This part need refactor, since the submitted jobs are not actually "running". The run function doesn't take effect. After sync with liuzhe, it may be better to add this test in IT. Maybe I should arrange an meeting to discuss issues about this pr.
There was a problem hiding this comment.
in line 91, you already run the training service, why it does not take effect? @liuzhe-lz @chicm-ms
There was a problem hiding this comment.
I add a generate_logfile function in mockedTrial.py to generate mock logfiles. However, these files seems not generated after run().
| fs.rmdirSync(path.join(rootDir, 'trials')) | ||
|
|
||
| await localTrainingService.cancelTrialJob(jobDetail.id); | ||
| }).timeout(20000); |
There was a problem hiding this comment.
this is testing training service, do we need to also test nnimanager or restserver?
There was a problem hiding this comment.
This UT for getTrialLog looks OK for me, we only want to test getTrialLog here, we do not care about other parts.
| @@ -72,6 +74,38 @@ describe('Unit Test for LocalTrainingService', () => { | |||
| chai.expect(jobDetail.status).to.be.equals('USER_CANCELED'); | |||
| }).timeout(20000); | |||
|
|
|||
| it('Get trial log', async () => { | |||
| // set meta data | |||
| const trialConfig: string = `{\"command\":\"python3 mockedTrial.py\", \"codeDir\":\"${localCodeDir}\",\"gpuNum\":0}` | |||
There was a problem hiding this comment.
I am wondering whether this mockedTrial.py will cause some conflicts regarding log content. It may generate log which make the log file content is not as expected.
There was a problem hiding this comment.
How about change the command to sleep 5 ?
There was a problem hiding this comment.
fixed. now use the original trialConifig in line 23
(cherry picked from commit 10c177c)
No description provided.