[serve][doc] Add async video-indexing tutorial with SageMaker comparison - #65225
[serve][doc] Add async video-indexing tutorial with SageMaker comparison#65225harshit-anyscale wants to merge 1 commit into
Conversation
6391f8e to
718c92d
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces an asynchronous video indexing service built on Ray Serve's task-consumer APIs, complete with a benchmark harness comparing its performance and autoscaling against Amazon SageMaker Asynchronous Inference. The codebase includes deployments for ingress, consumer, and encoder services, alongside utilities for video chunking, S3 storage, and idempotency. The review feedback highlights several critical issues: an autoscaling misconfiguration where target ongoing requests exceed the maximum allowed, a generic exception handler in S3 head operations that masks errors, concurrent resource deletion issues in the SageMaker deployment script, a hardcoded region in the Dockerfile base image, potential index errors when parsing video streams, a race condition in Redis RTT measurement, and a missing check for the S3 bucket environment variable.
718c92d to
a7c23c7
Compare
dstrodtman
left a comment
There was a problem hiding this comment.
Docs review: recommend restructuring before a line-level pass
Posted by Douglas Strodtman (@dstrodtman), Anyscale documentation team, reviewing as a @ray-project/ray-docs code owner on /doc/source/serve/ (.github/CODEOWNERS:76). Claude Code assisted in producing this review, per the AI-assistance disclosure in this repo's AGENTS.md. I read every line below and stand behind it.
Reviewed at a7c23c7f. The Gemini and Cursor comments on encoder.py and ingress.py were written against 718c92db; I checked both against current head and they're already fixed. Nothing here reopens them.
I want to be straightforward with you, Harshit, because I think it saves us both a round: I started a line-level style pass on this and stopped, because fixing the prose sentence by sentence would imply the page is close to mergeable, and I don't think it is. The engineering underneath looks solid and the SageMaker comparison is genuinely interesting material. But README.md isn't in the shape of a Ray documentation page, and I'd rather flag that once than send you 24 suggestions that leave the underlying problem in place.
What's missing relative to the tutorial shape
Compare doc/source/serve/tutorials/object-detection.md, which is the standard structure in this directory:
- A stated learning objective. The reference tutorial opens with what you'll build and what you'll learn. This page opens with a description of a service that already exists.
- A walkthrough of the feature it exists to demonstrate. This is the part I'd most want addressed. The page is a tutorial for the task-consumer async-inference APIs, but it never shows or explains how you declare a consumer, what the adapter config does, or how
AsyncInferenceAutoscalingPolicygets wired in. Those live indeployments/and the page only points at the directory. A reader can clone this and run it. They can't learn to build it, and that's the job of a tutorial. - Code in the page, from a tested source. Every snippet here is retyped prose. The reference tutorial pulls from
doc_code/with{literalinclude}. - A consistent register. Much of the writing is telegraphic in a way that reads as notes rather than documentation: "pulls tasks, downloads + chunks the video, calls the encoder, writes embeddings."
bench/sagemaker/README.mdis further along the same axis. - Scope. Roughly a third of the page is a competitive benchmark against SageMaker. That's positioning material, and I don't think it belongs in the same page as a how-to, if it belongs in
doc/at all.
There's also a "Repository layout" section, which no other page under doc/source/ has, because docs readers aren't browsing a directory tree.
Two verified errors, and why they matter here
Both are inline. Neither is a style nit:
Python 3.9+in the prerequisites. Ray requires>=3.10(python/setup.py:834).- "the consumer scales 1→4 [...] returns to 1" contradicts
consumer.py:39, which setsmin_replicas=2. The prose, the chart PNG, and the committed config disagree.
I'm raising these as evidence rather than as fixes. Both are the kind of error that surfaces the first time anyone runs the example, which brings me to the next part.
Nothing here is wired up for testing
The PR adds roughly 2,200 lines of Python under doc/source/. As far as I can trace, none of it runs anywhere:
- It isn't under
doc/source/serve/doc_code/, the only Serve path bazel executes (py_test_run_all_subdirectory,doc/BUILD.bazel:305). - No page pulls it in with
{literalinclude}. - There's no
ci/directory, so theall_examples_ci_configsfilegroup atdoc/BUILD.bazel:797(globbingsource/serve/tutorials/**/ci/{aws,gce}.yaml) doesn't see it. - There's no entry in
release/release_tests.yaml.
video-analysis, the closest precedent and a sibling directory, is wired end to end:
video-analysis |
this PR | |
|---|---|---|
| Executable artifact | README.ipynb |
none |
| Test driver | ci/tests.sh (converts the notebook via ci/nb2py.py, runs it) |
none |
| Compute configs | ci/aws.yaml, ci/gce.yaml |
none |
| Release test | video_analysis_pipeline, weekly, team: serve (release_tests.yaml:5041) |
none |
| Gallery entry | examples.yml:176 |
none |
That example keeps README.ipynb as the runnable source of truth and README.md as the rendered page, which is also why conf.py:328 excludes video-analysis/*.ipynb from the Sphinx build.
So there are two established options, and either one solves the code-linking problem and the testing problem together:
- Move the runnable parts to
doc/source/serve/doc_code/and{literalinclude}them into the page. - Follow the
video-analysislayout: a notebook or driver script, aci/directory, and a weekly release test. Probably the better fit here given the GPU, Redis, and S3 dependencies.
The page currently has no path to it
orphan: true keeps README.md out of the toctree and suppresses the fail_on_warning build error, but the PR doesn't add an entry to doc/source/serve/examples.yml, which is what the gallery directive in doc/source/custom_directives.py reads to build the Serve examples grid. The page builds and publishes at /serve/tutorials/video-indexing/README.html and appears in site search, but nothing in the Ray docs links to it. video-analysis is orphan: true and registered at examples.yml:176.
AGENTS.md disclosure
Ray's AGENTS.md requires PR descriptions for AI-assisted work to state three things: why the work isn't duplicating an existing issue or PR, the test commands run and their results, and that AI assistance was used. The description currently states none of the three. Worth updating either way, since the middle item is the open question above.
For the record on scope: that's a PR-description requirement, not a page-content one. I checked AGENTS.md, doc/.claude/CLAUDE.md, agent-development.md, and the style guide on current master, and there's no convention for disclosing AI assistance in rendered pages. Nothing needs to change in the Markdown.
What I'd suggest next
I don't think this needs to be thrown away. The system is real, the benchmark is real, and the assets are good. My suggestion:
- Decide what the page is for. If it's "how to build an async inference service on Ray Serve," the SageMaker benchmark is a separate artifact and probably a separate page or a blog post.
- Restructure around teaching the task-consumer API, with the code moved somewhere it can be executed and pulled in with
{literalinclude}. - Register it in
examples.ymlso readers can find it. - Then I'll do a full line-level style and grammar pass. I have one mostly written and I'm happy to run it the moment the structure settles.
@abrarsheikh, flagging you on the Serve side, specifically on whether a benchmark reproduction harness belongs under doc/source/ at all, and which of the two testing patterns you'd want here.
Happy to pair on the restructure if that's faster than review rounds. This is a real gap in the Serve docs and I'd like to see it land well.
| ## Repository layout | ||
|
|
||
| ``` | ||
| video-indexing/ | ||
| ├── app.py # composes the three deployments (import path: app:app) | ||
| ├── app_matched.py # monolith variant, used for the SageMaker comparison | ||
| ├── constants.py # queue names, model, env-var config | ||
| ├── config.yaml # local `serve run` config | ||
| ├── services.example.yaml # Anyscale deploy config template | ||
| ├── Containerfile # service image (Ray nightly + ffmpeg + deps) | ||
| ├── requirements.txt | ||
| ├── deployments/ # the deployments + shared config | ||
| │ ├── ingress.py consumer.py encoder.py processor.py | ||
| ├── utils/ # embedding_store, idempotency, s3, video helpers | ||
| └── bench/ # benchmark harness | ||
| ├── phased_load.py autoscale_poll.py e2e_latency.py | ||
| └── sagemaker/ # SageMaker Async comparison (deploy, load, image build) | ||
| ``` |
There was a problem hiding this comment.
A "Repository layout" section is the clearest single signal that this is a repository README rather than a docs page. Nothing else under doc/source/serve/tutorials/ has one, because a reader on docs.ray.io isn't browsing a directory.
It's also the only thing connecting the page to the 2,200 lines of Python the PR ships beside it. Ray has two patterns for that link and both also get the code tested. See the review body.
|
|
||
| ## Prerequisites | ||
|
|
||
| - Python 3.9+, `ffmpeg` on PATH, and `pip install -r requirements.txt`. |
There was a problem hiding this comment.
Verified wrong. python/setup.py:834 sets python_requires=">=3.10", and the classifiers list 3.10 through 3.14. A reader on 3.9 follows this prerequisite and then fails at pip install.
Flagging it here rather than suggesting a one-word fix because it's evidence for the review body: this is the kind of error that any executed version of this example would have caught immediately.
There was a problem hiding this comment.
Fixed. Corrected the prerequisite to Python 3.10+ to match python/setup.py (python_requires=">=3.10").
| The consumer scales on **Redis queue depth** via the stock | ||
| `AsyncInferenceAutoscalingPolicy` (polls the broker, combines queue length with | ||
| in-flight requests). The encoder scales on its own GPU load. Under a 500-video | ||
| burst the consumer scales 1→4 as the backlog builds, drains it, then returns to 1: |
There was a problem hiding this comment.
Verified mismatch against the code in this PR. The prose says the consumer "scales 1→4 [...] then returns to 1," but deployments/consumer.py:39 sets min_replicas=2. The consumer can't return to 1 replica.
So the prose, the chart in assets/queue_len_vs_replicas.png, and the committed config disagree with each other. I can't tell from the outside which one is right, which is roughly the problem: there's no executed path that would force them to agree.
Also worth a look: "The encoder scales on its own GPU load" reads as a distinct autoscaling signal, but encoder.py autoscales on target_ongoing_requests, which is a proxy for GPU load rather than a GPU metric.
There was a problem hiding this comment.
Fixed by aligning the code to the chart and prose. VideoIndexConsumer now uses min_replicas=1 (was 2), so the committed config, the queue_len_vs_replicas.png chart, and the "returns to 1" prose all agree. Thanks for catching it, this was the code diverging from the run the chart came from.
| ## Caveats | ||
| - **CloudWatch resolution ~1 min** -> coarser autoscaling traces than our ~3s poller. | ||
| - **Async invoke API has TPS/concurrency quotas** -> extreme rates (1000-3000/s) may be | ||
| capped by the API, not the backend; focus SageMaker on realistic rates + the | ||
| autoscaling/cost/throughput story (or request a TPS increase). | ||
| - **SageMaker instance-hours carry a managed premium** over raw g4dn - measure and report it. | ||
| - Autoscaling is instance-granular via Application Auto Scaling + CloudWatch alarms | ||
| (minutes of lag), vs Ray Serve's finer/faster replica autoscaling. |
There was a problem hiding this comment.
These caveats are notes from the person who ran the benchmark, not guidance for someone reproducing it. "measure and report it" and "focus SageMaker on realistic rates + the autoscaling/cost/throughput story" instruct the author, not the reader.
That's true of this file generally, which is why the review body suggests it probably shouldn't be a published page in its current form.
a7c23c7 to
be4edb0
Compare
be4edb0 to
3bfcfbb
Compare
| }], | ||
| AsyncInferenceConfig={ | ||
| "OutputConfig": {"S3OutputPath": a.s3_output, "S3FailurePath": failure}, | ||
| "ClientConfig": {"MaxConcurrentInvocationsPerInstance": a.max_concurrency_per_instance}, |
There was a problem hiding this comment.
SageMaker worker concurrency mismatch
Medium Severity
SAGEMAKER_MODEL_SERVER_WORKERS is hardcoded to 2 while MaxConcurrentInvocationsPerInstance defaults to 4 to match Ray’s max_ongoing_requests=4. Each model-server worker typically handles one request at a time, so SageMaker only runs two inferences in parallel per instance and the claimed 4-vs-4 comparison is not apples-to-apples.
Reviewed by Cursor Bugbot for commit 3bfcfbb. Configure here.
3bfcfbb to
5f3ea34
Compare
| max_replicas_per_node=1, | ||
| autoscaling_config=AutoscalingConfig( | ||
| min_replicas=6, | ||
| max_replicas=8, |
There was a problem hiding this comment.
Matched ingress needs six nodes
Medium Severity
Matched IndexingIngress sets min_replicas=6 with max_replicas_per_node=1, so it needs at least six nodes. The reproduce path copies services.example.yaml for a 4× T4 comparison, where only four nodes can host ingress replicas, leaving the matched app unable to become healthy.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5f3ea34. Configure here.
e755b0a to
103965e
Compare
Add an asynchronous inference example built on Ray Serve's task-consumer APIs: the client submits a video and gets a task id immediately, the work runs in the background off a Redis queue with queue-depth autoscaling, and embeddings are written to S3. Includes a benchmark harness comparing the service against Amazon SageMaker Asynchronous Inference under a matched flood. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: harshit <harshit@anyscale.com>
103965e to
9e296e8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Reviewed by Cursor Bugbot for commit 9e296e8. Configure here.
| out[label] = "flushed" | ||
| except Exception as e: | ||
| out[label] = repr(e) | ||
| return out |
There was a problem hiding this comment.
Unauthenticated Redis flush endpoint
Medium Severity
POST /admin/flush calls Redis flushdb on the broker and backend with no authentication, and services.example.yaml disables Serve query auth. Anyone who can reach the service URL can wipe the task queue and result backend.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 9e296e8. Configure here.


Summary
Adds a new Ray Serve tutorial: an asynchronous video-indexing service built on the task-consumer (async inference) APIs. A client submits a video and gets a task id immediately; the work runs in the background off a Redis queue, the consumer pool autoscales on queue depth, frames are embedded with SigLIP on a GPU, and the vectors are written to S3.
The example is self-contained under
doc/source/serve/tutorials/video-indexing/:app.pycomposes three deployments (IndexingIngress,VideoIndexConsumer,VideoEncoder) fromdeployments/.serve run+ Anyscale), submitting a request, and observing queue-depth autoscaling.bench/reproduces a flood load test, andbench/sagemaker/reproduces a matched comparison against Amazon SageMaker Asynchronous Inference (same hardware and flood, backlog-based autoscaling).Related issue number
N/A.
Not a duplicate
I searched open PRs and issues under Serve docs for
async inference,video indexing, andtask consumer; there is no other PR or issue adding an async-inference / task-consumer tutorial. This is the only one.Tests run
python -m ruff check --select F,E9andpython -m py_compileon the example's Python: pass.buildkite/premerge(lint) and the Read the Docs docs build both pass.g4dn.xlarge) plus a matched Amazon SageMaker Async endpoint, then drove the ~20-minute flood of ~67k videos. Result: 0 failed requests and an empty dead-letter queue on both sides.Transparency: the example is not yet wired into Ray's automated CI or release tests (
doc_code/execution, or avideo-analysis-styleci/directory plus a weekly release test). Adding that is in progress per the docs review on this PR.AI assistance
This PR was prepared with AI assistance (Claude Code). I have reviewed every changed line and run the checks above.