Enable deployment on Serve of functions that take no parameters #19708
Conversation
| @@ -884,6 +884,17 @@ class NegativeMaxQueries: | |||
| Base.options(max_concurrent_queries=-1) | |||
|
|
|||
|
|
|||
| def test_deploy_function_no_params(serve_instance): | |||
There was a problem hiding this comment.
Is there a reason all the other tests pass in serve_instance as an argument but don't necessarily use it? I pattern-matched the other tests when I included it, but I don't think I need it in the function.
There was a problem hiding this comment.
The serve_instance is a pytest fixture that creates a shared instance of ray + serve locally and allows all of the tests to connect to it. The actual argument itself may not be used, it's basically just specifying that the shared instance should be connected to before running this test.
| @@ -884,6 +884,17 @@ class NegativeMaxQueries: | |||
| Base.options(max_concurrent_queries=-1) | |||
|
|
|||
|
|
|||
| def test_deploy_function_no_params(serve_instance): | |||
There was a problem hiding this comment.
The serve_instance is a pytest fixture that creates a shared instance of ray + serve locally and allows all of the tests to connect to it. The actual argument itself may not be used, it's basically just specifying that the shared instance should be connected to before running this test.
| @@ -884,6 +884,17 @@ class NegativeMaxQueries: | |||
| Base.options(max_concurrent_queries=-1) | |||
|
|
|||
|
|
|||
| def test_deploy_function_no_params(serve_instance): | |||
There was a problem hiding this comment.
nit: these tests are quite similar, but conceptually I think this fits in test_api.py better, could you move it there?
| def d(): | ||
| return "Hello world!" |
There was a problem hiding this comment.
Can you also test a class that has a __call__(self): method?
| serve.start() | ||
| d.deploy() | ||
|
|
||
| assert requests.get("http://localhost:8000/d").text == "Hello world!" |
There was a problem hiding this comment.
Should also probably test the get_handle case here too
| runner_method = self.get_runner_method(request_item) | ||
| method_to_call = sync_to_async(runner_method) | ||
| result = None | ||
| if len(inspect.signature(runner_method).parameters) > 0: |
There was a problem hiding this comment.
Does this check need to be > 1 for class methods because of self? Where does the self arg get injected? It may work as-is because self is already bound in method_to_call. @simon-mo would probably know.
There was a problem hiding this comment.
it's already bound:
In [1]: class A:
...: def b(self):
...: pass
...:
In [2]: runner_method = A().b
In [3]: import inspect
In [4]: inspect.signature(runner_method).parameters
Out[4]: mappingproxy({})
edoakes
added
the
@author-action-required
… unit test for a Serve class
There was a problem hiding this comment.
lg, mostly about testing, can we add a few parametrized tests so that we cover:
- function deployment with no args
- class deployment with only "self"
- sync / async function for class deployment ?
For 3, example:
@serve.deployment
class A:
def __init__(self, input):
self.input = input
def __call__(self):
return self.input
where both functions can be sync or async.
…meters to check async behavior
|
|
Why are these changes needed?
Currently, functions that take no parameters can be deployed on Serve. However, when a request is made to the deployment over HTTP, the function will error out since it has no parameters. This PR allows functions with no parameters to successfully execute requests over HTTP without erroring out.
This PR was tested by adding a unit test called
test_deploy_function_no_paramstotest_deploy.pythat checks whether functions with no parameters run HTTP requests successfully. The PR also passed all other tests fromtest_deploy.py.Related issue number
Closes #12178
Checks
scripts/format.shto lint the changes in this PR.