-
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
[Serve] Return error string from deploy_serve_application
task
#36744
[Serve] Return error string from deploy_serve_application
task
#36744
Conversation
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
deploy_serve_application
taskdeploy_serve_application
task
@@ -846,6 +846,8 @@ def deploy_serve_application( | |||
name: application name. If specified, application will be deployed | |||
without removing existing applications. | |||
route_prefix: route_prefix. Define the route path for the application. | |||
Returns: | |||
Returns None if no error is raised. Otherwise, returns error message. | |||
""" | |||
try: | |||
from ray import serve |
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.
why are these delayed imports here btw?
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.
We can probably move call_app_builder_with_args_if_necessary
out of the task, but moving the others would cause the controller to depend on api.py
(to access serve.run()
and serve.build()
) which seems circular.
"working_dir": ( | ||
"https://github.com/ray-project/test_dag/" | ||
"archive/e552be913ffb7fb5e36b1e63d97f5c354d45e219.zip" | ||
), |
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.
let's use a local file instead please to avoid an external dependency
you can import from this file as from ray.serve.tests.test_controller import my_app
you can check it isn't serialized by raising an exception in __reduce__
or by including another non-serializable object as an attribute (and also may as well verify it isn't serializable using ray.cloudpickle
in the test case)
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.
Sounds good, I'll move it to the Ray repo. I can't put it in test_controller.py
though, since I need to raise the Exception
in the file itself (outside the deployment), in order for the error to get propagated back to the user.
I'll create a file in test_config_files
and import it from there.
Signed-off-by: Shreyas Krishnaswamy <shrekris@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, lint failing
The test failures are unrelated:
![]()
![]() |
@edoakes This PR is almost ready to merge. The |
The retry succeeded. @edoakes @architkulkarni This change is ready to merge. |
…ask (ray-project#36744)" This reverts commit 0fe1149. Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com>
…-project#36744) The controller runs the deploy_serve_application task to build and run the user's Serve app. If the task raises an error, the controller will try to deserialize it when it calls ray.get() on the task's reference. If the error contains a custom dependency, the deserialization will fail, and the controller will log an error about the deserialization failing instead of the actual error itself. This change catches any error in the deploy_serve_application task itself and returns it as a string to the controller. The controller then simply logs the string. Related issue number Closes ray-project#35677 and ray-project#35678. --------- Signed-off-by: Shreyas Krishnaswamy <shrekris@anyscale.com> Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
Why are these changes needed?
The controller runs the
deploy_serve_application
task to build and run the user's Serve app. If the task raises an error, the controller will try to deserialize it when it callsray.get()
on the task's reference. If the error contains a custom dependency, the deserialization will fail, and the controller will log an error about the deserialization failing instead of the actual error itself.This change catches any error in the
deploy_serve_application
task itself and returns it as a string to the controller. The controller then simply logs the string.Related issue number
Closes #35677 and #35678.
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.test_controller.py
.