-
Notifications
You must be signed in to change notification settings - Fork 7k
Remove deprecated dynamic generator from docs + public API #59228
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
base: master
Are you sure you want to change the base?
Remove deprecated dynamic generator from docs + public API #59228
Conversation
Signed-off-by: MaxVanDijck <[email protected]>
Signed-off-by: MaxVanDijck <[email protected]>
5c20711 to
5487426
Compare
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 effectively removes the deprecated dynamic generator from the public API and documentation, which is a good step towards simplifying the API. The changes in the documentation and option validation are clear and correct.
I've added one comment regarding a pre-existing bug in the validation logic for num_returns that could be fixed as part of this change to make it more robust.
Additionally, it seems that the tests for the dynamic generator feature have not been removed. For example, python/ray/tests/test_generators.py contains several tests that use num_returns="dynamic". These tests will likely fail with the changes in this PR. Please make sure to remove or update any related tests to ensure the CI passes and the codebase is clean.
| lambda x: None | ||
| if (x is None or x == "dynamic" or x == "streaming" or x >= 0) | ||
| if (x is None or x == "streaming" or x >= 0) |
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 validation logic for num_returns can raise a TypeError. If a string other than 'streaming' is passed (e.g., a typo like 'streamin'), the expression x >= 0 will be evaluated, causing a TypeError: '>=' not supported between 'str' and 'int'. This will crash the option validation.
To make the validation more robust, you should explicitly check the type of x before performing the comparison.
| lambda x: None | |
| if (x is None or x == "dynamic" or x == "streaming" or x >= 0) | |
| if (x is None or x == "streaming" or x >= 0) | |
| lambda x: None | |
| if (isinstance(x, int) and x >= 0) or x == "streaming" or x is None |
| (int, str, type(None)), | ||
| lambda x: None | ||
| if (x is None or x == "dynamic" or x == "streaming" or x >= 0) | ||
| if (x is None or x == "streaming" or x >= 0) |
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.
Bug: String comparison with integer causes TypeError in validation
The validation lambda for num_returns will raise a TypeError when a string other than "streaming" is passed (like the now-deprecated "dynamic"). The condition x >= 0 is evaluated after the string checks fail, but comparing a string to an integer in Python 3 raises TypeError: '>=' not supported between instances of 'str' and 'int'. The original code had x == "dynamic" checked first, which prevented this. Now, passing an invalid string value will crash with a TypeError instead of returning the user-friendly validation error message.
Signed-off-by: MaxVanDijck <[email protected]>
Signed-off-by: MaxVanDijck <[email protected]>
Description
Addresses 2 requirements from issue: #59155
Removes the public api usage of "dynamic" and removes documentation references to the dynamic generator
Related issues
Partial of #59155