Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[OTE] Comment out time.sleep(2) in EpochRunnerWithCancel #1174

Merged
merged 6 commits into from
Jul 20, 2022
Merged

[OTE] Comment out time.sleep(2) in EpochRunnerWithCancel #1174

merged 6 commits into from
Jul 20, 2022

Conversation

JihwanEom
Copy link
Contributor

@JihwanEom JihwanEom commented Jul 19, 2022 •
edited
Loading

This PR includes:

Summary

  • Comment out time.sleep(2) in EpochRunnerWithCancel for all tasks. (Deadlock is not worried for single-gpu)

@JihwanEom JihwanEom self-assigned this Jul 19, 2022
@JihwanEom JihwanEom requested a review from a team as a code owner July 19, 2022 06:35
@github-actions github-actions bot added the ALGO Any changes in OTX Algo Tasks implementation label Jul 19, 2022
@JihwanEom JihwanEom changed the title [OTE] Remove time.sleep(2) in EpochRunnerWithCancel [OTE] Comment out time.sleep(2) in EpochRunnerWithCancel Jul 19, 2022
goodsong81
goodsong81 suggested changes Jul 19, 2022
View reviewed changes
Copy link

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

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

Could you add comments?
ex) # TODO: uncomment this line or resolve root cause of deadlock issue if multi-GPUs need to be supported.

@JihwanEom
Copy link
Contributor Author

Could you add comments? ex) # TODO: uncomment this line or resolve root cause of deadlock issue if multi-GPUs need to be supported.

Revised. Thank you.

harimkang
harimkang approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@harimkang harimkang left a comment

Choose a reason for hiding this comment

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

LGTM,
I think there is no problem because the multi-gpu environment is not currently used, but I would like to proceed with the SC test. I would like you to check and comment on this before it is merged with SC.

@eunwoosh eunwoosh force-pushed the modify-mpa-templates-for-hpo branch from ae99810 to 6af3a80 Compare July 19, 2022 12:16
@github-actions github-actions bot added CLI Any changes in OTE CLI and removed CLI Any changes in OTE CLI labels Jul 20, 2022
goodsong81
goodsong81 approved these changes Jul 20, 2022
View reviewed changes
Copy link

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

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

LGTM

@JihwanEom JihwanEom merged commit e79eb08 into modify-mpa-templates-for-hpo Jul 20, 2022
@JihwanEom JihwanEom deleted the jeom/remove-sleep branch July 20, 2022 02:30
eunwoosh pushed a commit that referenced this pull request Jul 20, 2022
@JihwanEom @eunwoosh
[OTE] Comment out time.sleep(2) in EpochRunnerWithCancel (#1174)
87d06fe 
* Remove time.sleep(2) in EpochRunnerWithCancel
Co-authored-by: eunwoosh <eunwoo.shin@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goodsong81 goodsong81 goodsong81 approved these changes

@harimkang harimkang harimkang approved these changes

@chuneuny-emily chuneuny-emily Awaiting requested review from chuneuny-emily

@jaegukhyun jaegukhyun Awaiting requested review from jaegukhyun

@supersoob supersoob Awaiting requested review from supersoob

Assignees

@JihwanEom JihwanEom

Labels
ALGO Any changes in OTX Algo Tasks implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JihwanEom @goodsong81 @harimkang @eunwoosh