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

Add timeout for web_channel in trial_runner #2710

Merged
merged 19 commits into from
Jul 24, 2020

Conversation

SparkSnail
Copy link
Contributor

No description provided.

@ultmaster ultmaster requested a review from squirrelsc July 22, 2020 03:28
@scarlett2018 scarlett2018 mentioned this pull request Jul 22, 2020
66 tasks
self.client = client
nni_log(LogType.Info, 'WebChannel: connected with info %s' % url)
except asyncio.TimeoutError:
nni_log(LogType.Error, 'WebChannel: connect %s failed!' % url)
Copy link
Member

Choose a reason for hiding this comment

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

change the error message like below. It's more precise, and guide user to fix it.

WebChannel: connect to %s timeout! Please make sure NNIManagerIP configured correclty, and accessable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

try:
connect = asyncio.wait_for(websockets.connect(url), self.timeout)
self._event_loop = asyncio.get_event_loop()
client = self._event_loop.run_until_complete(connect)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to hang at this line in some cases? Is a timeout needed for event loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This self._event_loop.run_until_complete(connect) is for connect method, and connect has already handled timeout.

@SparkSnail SparkSnail requested a review from chicm-ms July 23, 2020 02:59
self.client = client
nni_log(LogType.Info, 'WebChannel: connected with info %s' % url)
except asyncio.TimeoutError:
nni_log(LogType.Error, 'connect to %s timeout! Please make sure NNIManagerIP configured correclty, and accessable.' % url)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: correclty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@SparkSnail SparkSnail merged commit 54fef7f into microsoft:master Jul 24, 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.

3 participants