[Docs] sky batch#9438
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive documentation for Sky Batch, including a quickstart guide and instructions for implementing custom InputReaders and OutputWriters. It also updates the documentation index and navigation. Key feedback includes the accidental inclusion of a local lock file (.claude/scheduled_tasks.lock) and a recommendation to refactor the reduce_results examples to be more memory-efficient, as the current implementations load all data into memory, contradicting the documentation's own performance warnings.
I am having trouble creating individual review comments. Click here to see my feedback.
.claude/scheduled_tasks.lock (1)
This file appears to be a local state or lock file from an AI tool (.claude/). It should not be committed to the repository. Please remove it and ensure .claude/ is added to the project's .gitignore.
docs/source/examples/batch/custom-formats.rst (255-257)
This implementation of reduce_results loads all batch results into memory on the controller by extending all_items. This directly contradicts the warning in the note at lines 169-171, which advises against loading all results into memory at once. For large datasets, this pattern will cause the controller (typically a small VM) to run out of memory. While YAML is difficult to stream, for a documentation example, it would be better to either use a format that supports streaming (like JSONL) or add a clear disclaimer that this specific approach is only suitable for small datasets.
docs/source/examples/batch/custom-formats.rst (386-388)
As noted in the previous example, this reduce_results implementation in the complete example also accumulates all results in memory. This is an anti-pattern for the controller as described in the warning at lines 169-171. Consider revising the example to use a more memory-efficient approach or highlighting the limitation.
|
/build-docs |
|
✅ ReadTheDocs build triggered for branch The documentation will be available at: https://docs.skypilot.co/en/sky-batch-doc/ |
Michaelvll
left a comment
There was a problem hiding this comment.
Thanks @cblmemo! LGTM
|
/build-docs |
|
❌ @cblmemo is not authorized to trigger ReadTheDocs builds. Please contact a maintainer. |
|
❌ Failed to trigger ReadTheDocs build for branch |
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)