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

Hotfix for v1.7.1 #2692

Merged
merged 8 commits into from
Jul 19, 2020
Merged

Hotfix for v1.7.1 #2692

merged 8 commits into from
Jul 19, 2020

Conversation

SparkSnail
Copy link
Contributor

No description provided.

@@ -122,7 +122,7 @@ export class OpenPaiEnvironmentService extends EnvironmentService {
break;
case 'SUCCEEDED':
case 'FAILED':
Copy link
Member

Choose a reason for hiding this comment

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

add a break above Failed, so that succeeded env won't fail experiment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

if (!amlClient) {
throw new Error('AML client not initialized!');
if (!amlClient) {
return Promise.reject('AML client not initialized!');
Copy link
Member

Choose a reason for hiding this comment

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

BTW, why throw error doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no try catch outside the function, throw this error directly will not be shown in web ui.

Copy link
Member

Choose a reason for hiding this comment

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

Did you try it? there are a lot of throw error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I tried it. If we only use throw Error without catch, the error information will only shown in nnictl log stderr instead of webui.

Copy link
Contributor

Choose a reason for hiding this comment

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

The throw new Error here should work without try catch because there is a catch in nnimanager and this promise is called from nnimanager. It works in other training service. Would you please check why it is not working?

@@ -98,8 +98,7 @@ export class AMLEnvironmentService extends EnvironmentService {
environment.setFinalStatus('SUCCEEDED');
break;
case 'FAILED':
environment.setFinalStatus('FAILED');
break;
return Promise.reject(`AML: job ${environment.jobId} is failed!`);
Copy link
Contributor

Choose a reason for hiding this comment

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

besides returning reject, do we need to set final status here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't need to set status for environment here. when return Promise.reject, the experiment become error status, and nniManager stops. this final status is not useful anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it

Copy link
Member

Choose a reason for hiding this comment

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

well, the status is not visible so far. But if we expose it to webui in future, it would be a hole.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@scarlett2018 scarlett2018 mentioned this pull request Jul 17, 2020
66 tasks
@SparkSnail SparkSnail merged commit 1118e90 into microsoft:v1.7.1 Jul 19, 2020
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.

4 participants