-
Notifications
You must be signed in to change notification settings - Fork 148
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
Simplified fix for issue 803 #839
Conversation
Signed-off-by: Constantin M Adam <cmadam@us.ibm.com>
from ray.util.actor_pool import ActorPool | ||
|
||
|
||
RAY_MAX_ACTOR_LIMIT = 10000 |
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.
Please consider using environment variable as below. This keep the default at 100 (as it is now) but then allows you to set an environment variable to increate the number of actors if needed
RAY_MAX_ACTOR_LIMIT = 10000 | |
RAY_MAX_ACTOR_LIMIT = int (os.environ.get("RAY_MAX_ACTOR_LIMIT", '100')) |
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 do not see a benefit from using an additional environment variable. First, setting 100 as the default means issue #803 will occur by default, requiring users to read the documentation, locate the issue, and define a new environment variable to fix it. Second, Ray already uses an environment variable to define the RAY_MAX_LIMIT_FROM_API_SERVER
constant here. Playing with this environment variable might allow us to change the max limit value, but I do not see what is the advantage of doing so.
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.
@touma-I : I can see that @dtsuzuku-ibm has opened an identical issue (#858) and a pull request (#859) with exactly the same fix as proposed 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.
Thanks @cmadam This is exactly what we need so we can just address the issue we are facing. I encourage you to consider the comment to use an environment variable though. This way we can use a default and if we want to overwrite the default, we can do so without rebuilding the code. Let me know what you think.
Signed-off-by: Constantin M Adam <cmadam@us.ibm.com>
Why are these changes needed?
This provides a simplified, minimal fix for issue #803
Related issue number (if any).
#803