-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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][Doc] Update docs about input schema, and json_request adapter #24191
Conversation
@@ -16,15 +16,15 @@ kernelspec: | |||
|
|||
# Deployment Graph | |||
|
|||
```{note} |
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.
trailing whitespace, hard to revert but good for code quality, so i kept these changes.
@@ -394,31 +420,21 @@ more info on these options. | |||
|
|||
Now we're done! The full example below covers the full example for you to try out. | |||
|
|||
```{code-cell} ipython3 | |||
:tags: [remove-cell] |
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 should show some of these so when people copy the full example from the web page, the code works end to end
# Each serve dag has a driver deployment as ingress that can be user provided. | ||
serve_dag = DAGDriver.options(route_prefix="/my-dag").bind(dag) | ||
serve_dag = DAGDriver.options(route_prefix="/my-dag", num_replicas=2).bind( |
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.
verified locally running this example works end to end. we should convert this to a Python file and run in CI (but in a separate PR)
```{note} | ||
We expect each DAG has a driver class implementation as root, similar to the example below. This is where HTTP ingress are configured and implemented. We provide a default `DAGDriver` to handle simple HTTP parsing, but in this example we put up a custom implementation. | ||
Serve provides a default DAGDriver implementation that accepts HTTP request and orchestrate the deployment graph execution. You can import it from `from ray.serve.drivers import DAGDriver`. | ||
|
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 we should also mention user has all the freedom they want to provide their own DAGDriver implementation as ingress just as if what they would do today, and what we did here is to facilitate holding dag instance by providing a default template.
Realistically i think there'll be decent # of users who start with our default implementation then add their own code to it rather than our DAGDriver covers everything they need ?
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.
it's in two paragraph below.
does our CI provide a default link to the documentation based on latest commit to review the whole webpage ? i remember seeing it from my last iteration of the documentation |
content looks good to me, do you want to change |
input_schema=json_request, | ||
) | ||
handle = serve.run(dag) | ||
assert ray.get(handle.predict.remote([1, 2])) == [1, 2] |
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.
can you try it with an example that passes multiple objects to dag.execute(1, 2, [3,4])
? I remember default DAGDriver will throw due to mismatched signature that lead to different behaviors between ray dag and serve dag regarding InputNode handling
input_schema=json_request, | ||
) | ||
handle = serve.run(dag) | ||
assert ray.get(handle.predict.remote([1, 2, [3, 4]])) == [1, 2, [3, 4]] |
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.
sorry i actually meant handle.predict.remote(1, 2, [3, 4])
so it's multiple args that will spawn InputAttributeNode
rather than dealing with one python object. You can see them in https://sourcegraph.com/github.com/ray-project/ray/-/blob/python/ray/experimental/dag/tests/test_input_node.py?L225
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.
This doesn't make sense for typical DAGDriver here. The input is still a single item because predict and http request has a single value representing the input. As you can see in the dag users can still write input[0] to index into input node.
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.
how about we use the same internal data wrapper as ray dag's execute ? https://sourcegraph.com/github.com/ray-project/ray/-/blob/python/ray/experimental/dag/input_node.py?L255
I understand eventually InputNode translate to one python object, but it's better if we can minimize the churn from user's dev flow if they find out they need to change input syntax / structure from ray core to serve dag. I'm not suggesting what we have now is already optimal, but they should be the same class.
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.
synced offline and agreed on doing *args **kwargs unpacking on default DAGDriver
Why are these changes needed?
This PR dives into more detail about the input schema type. In particular:
Related issue number
Closes #23484
Checks
scripts/format.sh
to lint the changes in this PR.