Skip to content

[Docs] sky batch#9438

Merged
cblmemo merged 4 commits into
masterfrom
sky-batch-doc
Apr 27, 2026
Merged

[Docs] sky batch#9438
cblmemo merged 4 commits into
masterfrom
sky-batch-doc

Conversation

@cblmemo

@cblmemo cblmemo commented Apr 23, 2026

Copy link
Copy Markdown
Collaborator

Tested (run the relevant ones):

  • Code formatting: install pre-commit (auto-check on commit) or bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: /smoke-test (CI) or pytest tests/test_smoke.py (local)
  • Relevant individual tests: /smoke-test -k test_name (CI) or pytest tests/test_smoke.py::test_name (local)
  • Backward compatibility: /quicktest-core (CI) or pytest tests/smoke_tests/test_backward_compat.py (local)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

medium

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)

medium

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)

medium

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.

@Michaelvll

Copy link
Copy Markdown
Collaborator

/build-docs

@github-actions

Copy link
Copy Markdown
Contributor

✅ ReadTheDocs build triggered for branch sky-batch-doc

The documentation will be available at: https://docs.skypilot.co/en/sky-batch-doc/

@Michaelvll Michaelvll left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cblmemo! LGTM

Comment thread docs/source/docs/index.rst Outdated
@cblmemo

cblmemo commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator Author

/build-docs

@github-actions

Copy link
Copy Markdown
Contributor

@cblmemo is not authorized to trigger ReadTheDocs builds. Please contact a maintainer.

@github-actions

Copy link
Copy Markdown
Contributor

❌ Failed to trigger ReadTheDocs build for branch unknown. Please check the workflow logs for details.

@cblmemo cblmemo enabled auto-merge (squash) April 27, 2026 01:09
@cblmemo cblmemo merged commit 9f56c2b into master Apr 27, 2026
21 checks passed
@cblmemo cblmemo deleted the sky-batch-doc branch April 27, 2026 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants