-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[RLlib] Replace ordinary pygame imports by try_imports #31332
[RLlib] Replace ordinary pygame imports by try_imports #31332
Conversation
from typing import Optional | ||
|
||
import numpy as np |
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.
Just curious, is this a new sorting convention? Three blocks: a) included python modules, b) other third-party imports, c) ray imports?
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.
The three categories you describe (and the formating changes here) conform with PEP-8 I belive.
-> https://peps.python.org/pep-0008/#imports
I think we should adhere at least to PEP-8. The google style guide is optional.
I use PyCharm's "optimize imports" feature to do these, but PyCharm also groups "import x as y" vs "import x" and separates them which makes for apparent missing alphabetical order.
rllib/env/utils.py
Outdated
error: Whether to raise an error if pyspiel cannot be imported. | ||
|
||
Returns: | ||
The tfp module. |
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.
-> "pyspiel", not tfp
rllib/env/utils.py
Outdated
The tfp module. | ||
|
||
Raises: | ||
ImportError: If error=True and tfp is not installed. |
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.
-> "pyspiel" instead of tfp
import re | ||
|
||
import numpy as np | ||
from open_spiel.python.rl_environment import Environment |
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.
Dumb question: what was the difference between open_spiel
and pyspiel
again? Would we need to return the open_spiel
package as well from try_import_pyspiel()
?
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.
open spiel uses pygame, which is a game engine with a python interface.
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.
They are separate packages though.
We list pygame in our test requirements and open_spiel nowhere.
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.
I have added open_spiel to this PR with the same behaviour!
import sys | ||
|
||
import numpy as np | ||
from open_spiel.python.rl_environment import Environment |
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.
same question as above.
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. Just a few questions on pyspiel vs open_spiel.
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
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.
LGTM.
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
…oject#31332) Signed-off-by: Andrea Pisoni <andreapiso@gmail.com>
Signed-off-by: Artur Niederfahrenhorst artur@anyscale.com
Why are these changes needed?
Replaces imports of pygame with try_imports, akin to the ones we use for frameworks.
This helps us output a more informative import error.
Related issue number
#31321
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.