-
Notifications
You must be signed in to change notification settings - Fork 7k
[Bugfix][rllib]: Correct typo and consistency in pyspiel import error message (#53841) #54618
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
[Bugfix][rllib]: Correct typo and consistency in pyspiel import error message (#53841) #54618
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.
Summary of Changes
Hello @Flink-ddd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a minor but important issue by correcting an erroneous import error message. The change ensures that users receive accurate guidance for installing the correct dependency when a specific module fails to import, thereby improving the overall developer experience and reducing potential confusion.
Highlights
- Error Message Correction: Corrected a typo and inconsistency within the
ImportErrormessage for thepyspielmodule. The message previously incorrectly referred to 'Pygame' instead of 'pyspiel'. - Improved User Guidance: Updated the suggested installation command provided in the
ImportErrormessage frompip install pygametopip install pyspiel, ensuring users receive accurate instructions for resolving the missing dependency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request correctly fixes a typo in an error message for the optional pyspiel dependency, improving clarity for users. My review includes a suggestion to further refine the error message to be more explicit about pyspiel being an optional dependency, which would make it even more helpful.
rllib/env/utils/__init__.py
Outdated
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 fixing this typo! The change is correct.
For even better clarity, I suggest rephrasing the error message to avoid the potentially confusing statement that pyspiel is "not a dependency". While it's not a core dependency, it is required for certain features, making it an optional dependency.
This alternative wording is more direct and clearly communicates the nature of the dependency to the user.
A similar message exists for open_spiel in this file, which could also benefit from this clarification for consistency.
| "Could not import pyspiel! pyspiel is not a dependency of RLlib " | |
| "and RLlib requires you to install pyspiel separately: " | |
| "`pip install pyspiel`." | |
| "Could not import pyspiel! pyspiel is an optional dependency of RLlib " | |
| "and must be installed separately. You can install it with: " | |
| "`pip install pyspiel`." |
8663107 to
a25c718
Compare
a25c718 to
e3c3f94
Compare
|
Hi @cszhu , I noticed that buildkite/microcheck keeps failing for this PR (build #20431 etc). My change was just a small typo fix for the pyspiel import error message (from pygame to pyspiel), is this related to general instability in CI or inherent instability in these specific tests themselves? Any insight or manual rerun and I would be greatly appreciated. Thanks! |
|
Hello @Flink-ddd , thanks for opening a PR to help contribute to Ray :) I took a peek at the microcheck, I think it's likely not due to the changes in your CR since you just edited an error message... Unless the tests are checking specifically for that error message. |
28f0fbb to
5b57803
Compare
…#53841) Signed-off-by: Vensenmu <[email protected]>
5b57803 to
546ba63
Compare
|
Hi @cszhu , Thank you very much to reply me, buildkite/microcheck already passed, but the buildkite/premerge wait a long time, what's question is it? |
|
Hi, the |
|
Hi, I see, Thank you very much for you reply. |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
I've checked through the tests and can't see any would cause an issue from fixing this error message. |
|
Thank you! I don't have merge perms but I will let the team know and get back to you 🫡 |
|
Hi @pseudo-rnd-thoughts , Thank for your approval, btw about this PR what time can be merge to master? |
|
@Flink-ddd I don't have permissions to merge but will get someone who can |
|
Hi @pseudo-rnd-thoughts , Thanks for your help and for finding someone to merge this! Thanks again! |
…age (ray-project#53841) (ray-project#54618) Signed-off-by: Aydin Abiar <[email protected]>
…age (ray-project#53841) (ray-project#54618) Signed-off-by: YK <[email protected]>
This PR resolves an issue where the
ImportErrormessage forpyspielinrllib/env/utils/__init__.py(line 37-39) contained a typo and inconsistent information.Specifically, it changes:
"Pygame is not a dependency of RLlib"to"Pyspiel is not a dependency of RLlib"pip install pygame" to "pip install pyspiel"This ensures that users receive accurate guidance for installing the correct dependency (
pyspiel) when the import fails.Closes #53841
Note
Fixes the
try_import_pyspielImportError message to referencepyspieland suggest installingopen_spiel.try_import_pyspielImportError text to correctly referencepyspieland guide installation viapip install open_spiel.Written by Cursor Bugbot for commit 119e42e. This will update automatically on new commits. Configure here.