-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[rllib] Fix AlgorithmConfig validation when using OfflinePreLearner #55416
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
Conversation
Signed-off-by: John Pertoft <[email protected]>
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 @johnPertoft, 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!
I've addressed an issue where the AlgorithmConfig validation was failing when an OfflinePreLearner was being used. The problem stemmed from an incorrect attribute being checked during the validation process, leading to erroneous validation failures.
Highlights
- AlgorithmConfig Validation Fix: I've resolved a bug where the AlgorithmConfig validation incorrectly failed when an OfflinePreLearner was configured.
- Attribute Correction: The validation logic in _validate_offline_settings was updated to correctly reference self.offline_eval_runner_class instead of the previously used self.prelearner_class when checking for OfflineEvaluationRunner inheritance.
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 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 or fill out our survey 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 validation bug in AlgorithmConfig by checking self.offline_eval_runner_class instead of self.prelearner_class. I've added a comment pointing out a related issue where the evaluation() method seems to set a different property (self.offline_eval_runner_cls), which would make this new validation ineffective. Fixing that inconsistency will make this change fully effective.
| if self.offline_eval_runner_class and not issubclass( | ||
| self.offline_eval_runner_class, OfflineEvaluationRunner | ||
| ): |
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.
This change correctly fixes the validation logic to check self.offline_eval_runner_class. However, there appears to be a related issue in the evaluation() method. It seems to set self.offline_eval_runner_cls (with a _cls suffix) instead of self.offline_eval_runner_class when the offline_eval_runner_class parameter is provided.
Because of this, the validation here might not be effective, as self.offline_eval_runner_class may not be set correctly through the public API.
To ensure this validation works as intended, please consider correcting the property assignment in the evaluation() method (around line 2992) to use self.offline_eval_runner_class.
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.
@johnPertoft seems to me like this should also be changed to class instead of cls
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, I've updated it to include this change as well.
Signed-off-by: John Pertoft <[email protected]>
|
Closing this as it has been fixed in #55693 |
Why are these changes needed?
When using an OfflinePreLearner the algorithm config validation fails.
Related issue number
#55415
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.