[DNM] Syncing deployment serve llm from published s3 -> ray doc build#61124
[DNM] Syncing deployment serve llm from published s3 -> ray doc build#61124elliot-barn wants to merge 4 commits intomasterfrom
Conversation
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request introduces sphinx-collections to sync documentation examples from an S3 bucket, which is a good strategy for keeping content up-to-date. The changes include updating dependency configurations, adding the new tool, and setting up the collection process in conf.py. My review focuses on a potential issue in the dependency configuration that could break builds, and offers suggestions to improve the new script's clarity and memory efficiency.
| output: python/deplocks/ci/serve_base_depset_py${PYTHON_VERSION}.lock | ||
| append_flags: | ||
| - --extra-index-url https://download.pytorch.org/whl/cpu | ||
| - --index https://download.pytorch.org/whl/cpu |
There was a problem hiding this comment.
Using --index (an alias for --index-url) will replace the default PyPI index with the PyTorch index. This will likely cause dependency resolution to fail for any packages not hosted on the PyTorch index. If the intention is to add this as a secondary index, this should be --extra-index-url.
- --extra-index-url https://download.pytorch.org/whl/cpu| with urlopen(url) as resp: | ||
| zip_bytes = io.BytesIO(resp.read()) |
There was a problem hiding this comment.
This implementation reads the entire zip file into memory before creating a BytesIO object. For large zip files, this could lead to high memory consumption. A more memory-efficient approach would be to stream the download to a temporary file on disk and then extract from there. For example, using tempfile and shutil.copyfileobj.
| # Don't wipe the target before build — other docs may co-exist in parent dirs. | ||
| collections_clean = True |
There was a problem hiding this comment.
The comment on line 135 contradicts the setting on line 136. collections_clean = True will wipe the target directory before the build, but the comment says "Don't wipe the target...". If the intention is to have a clean state from the S3 artifact (which seems likely given the file deletions in this PR), the comment should be updated to reflect this.
| # Don't wipe the target before build — other docs may co-exist in parent dirs. | |
| collections_clean = True | |
| # Wipe the target directory before build to ensure a clean state. | |
| collections_clean = True |
| } | ||
|
|
||
| # Don't wipe the target before build — other docs may co-exist in parent dirs. | ||
| collections_clean = True |
There was a problem hiding this comment.
Comment contradicts code: collections_clean set to wrong value
Medium Severity
The comment on line 135 says "Don't wipe the target before build" but collections_clean is set to True, which does exactly that — it wipes all configured target locations at the start of each build. According to the sphinx-collections documentation, setting collections_clean = True causes all configured target directories to be wiped before collection runs. To match the stated intent of not wiping, this value needs to be False.
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>


[DO NOT MERGE: requires a production deploy before merging]
Using sphinx collections to sync deployment serve llm example from published s3 bucket to our doc build.
Tested locally by running
make local:Screenshots (8)