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.
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
Fix GPU run for PyTorch Ignite and Lightning examples. #1444
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR! The change looks good to me.
Please check two small comments.
@@ -37,7 +37,6 @@ | |||
|
|||
import optuna | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keisuke-umezawa @hvy @HideakiImamura
I think we had a coding style convention to put two blank lines after library imports, but I cannot find it in the current guide. Can we accept this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I did not know that. We have existing code where there's a single blank line, e.g. similar to this case where variables are declared (e.g. logger objects) or conditions using try_import
. I thought that the "two blank lines rule" stemmed from PEP-8 https://www.python.org/dev/peps/pep-0008/#blank-lines and that it's okay as long as we follow
Surround top-level function and class definitions with two blank lines.
(But I do think that it's confusing having the style checkers (black
, hacking
) accept either here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hvy Thank you for your comment.
I thought that the "two blank lines rule" stemmed from PEP-8 https://www.python.org/dev/peps/pep-0008/#blank-lines
I understand. I'd like to accept this change according to PEP-8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your update. LGTM.!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes, changes LGTM!
Let me slightly modify the title to match the release note format https://github.com/optuna/optuna/blob/master/CONTRIBUTING.md#title. |
Motivation
The pytorch ignite and lightning example failed to run using GPU.
Description of the changes
This PR fix the run with GPU.