-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[AIR] Remove PredictorDeployment from examples #37457
[AIR] Remove PredictorDeployment from examples #37457
Conversation
c5e78dc
to
b039fc3
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.
Generally LGTM, and honestly cleaner than I expected 😄
Before merging:
- Can you add a PR description for this change?
- Can you verify the CI test failures are unrelated?
@@ -159,70 +159,78 @@ def send_request(**requests_kargs): | |||
|
|||
|
|||
def test_simple_adder(serve_instance): |
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.
Do we still need these tests? Is there anything that is being tested here that is not being covered by Predictor
and Serve unit tests?
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 think it's worthwhile to test the combination of predictor deployment + serve in CI somewhere. I don't think we have anything similar in the serve unit tests at the moment.
@@ -159,70 +159,78 @@ def send_request(**requests_kargs): | |||
|
|||
|
|||
def test_simple_adder(serve_instance): |
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 think it's worthwhile to test the combination of predictor deployment + serve in CI somewhere. I don't think we have anything similar in the serve unit tests at the moment.
predictor_cls=AdderPredictor, | ||
@serve.deployment | ||
class AdderDeployment: | ||
def __init__(self, checkpoint): |
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.
type hints would be very nice here :)
This PR removes the AIR PredictorDeployment from examples and replaces them with Ray Serve deployments directly. This is more explicit and often time simpler and more understandable. It also reduces the number of ways in which things are done according to the Zen of Python `There should be one -- and preferably only one -- obvious way to do it.` Signed-off-by: NripeshN <nn2012@hw.ac.uk>
This PR removes the AIR PredictorDeployment from examples and replaces them with Ray Serve deployments directly. This is more explicit and often time simpler and more understandable. It also reduces the number of ways in which things are done according to the Zen of Python `There should be one -- and preferably only one -- obvious way to do it.` Signed-off-by: harborn <gangsheng.wu@intel.com>
This PR removes the AIR PredictorDeployment from examples and replaces them with Ray Serve deployments directly. This is more explicit and often time simpler and more understandable. It also reduces the number of ways in which things are done according to the Zen of Python `There should be one -- and preferably only one -- obvious way to do it.`
This PR removes the AIR PredictorDeployment from examples and replaces them with Ray Serve deployments directly. This is more explicit and often time simpler and more understandable. It also reduces the number of ways in which things are done according to the Zen of Python `There should be one -- and preferably only one -- obvious way to do it.` Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
This PR removes the AIR PredictorDeployment from examples and replaces them with Ray Serve deployments directly. This is more explicit and often time simpler and more understandable. It also reduces the number of ways in which things are done according to the Zen of Python `There should be one -- and preferably only one -- obvious way to do it.` Signed-off-by: Victor <vctr.y.m@example.com>
Why are these changes needed?
This PR removes the AIR PredictorDeployment from examples and replaces them with Ray Serve deployments directly. This is more explicit and often time simpler and more understandable. It also reduces the number of ways in which things are done according to the Zen of Python
There should be one -- and preferably only one -- obvious way to do it.
.Related issue number
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.