[Doc] Fix RTD language toggle paths#235
Conversation
…rt spurious dist.gather_object patch arguments.py (415→361 lines): - Trim module docstring and inline comments throughout - Slim SKIPPED_DESTS to names-only (remove per-entry rationale) - Add mutual-exclusion assertions in validate_args mirroring slime (vllm_config / prefill_num_servers / rollout_external conflict checks) - Note: _make_add_argument_wrapper stays public (5 unit tests reference it by name); validate_args intentionally omits vllm_tp_size (pinned by test — global TP broke heterogeneous per-group engines, per-engine resolver owns it) reloadable_process_group.py: - Drop dist.gather_object monkey-patch (added by PR #22, never in slime; unused in vime — callers explicitly use all_gather_object to stay on the patched path) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reverts: - PR #26: remove pre-commit gate job + needs: pre-commit on all jobs; simplify always/cpu if-branch back to slime's single-level structure - PR #110: remove e2e-test-unit job (pytest tests/unit tests/utils) Syncs from slime (non-PR-#150): - PR trigger: add opened, reopened to pull_request types - docker run: add --pull=always to GPU job (loop + e2e-test-changed) - CPU job pip install: add requests ray safetensors - Rename e2e-test-plugin-contracts → cpu-unittest / run-ci-cpu-unittest - Restore upstream comment text for push-to-main trigger Remaining diffs vs slime are all intentional: - vime translations (vllm_config, VIME_TEST_*, inferactinc image, vime_ci paths) - PR #150 test matrix changes (skipped per user instruction) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the language toggle script to support Read the Docs project paths, refactors and simplifies comments in the vLLM arguments module, adds argument validation checks, and removes a monkey-patch in the reloadable process group utility. Feedback highlights a potential regression from removing the dist.gather_object patch, an outdated comment in the language toggle script, and an opportunity to simplify the language URL construction logic.
| dist.all_gather = get_new_comm_function(dist.all_gather) | ||
| dist.all_gather_into_tensor = get_new_comm_function(dist.all_gather_into_tensor, "all_gather_into_tensor") | ||
| dist.all_gather_object = get_new_comm_function(dist.all_gather_object) | ||
| dist.gather_object = get_new_comm_function(dist.gather_object) |
There was a problem hiding this comment.
The monkey-patch for dist.gather_object has been removed. Was this removal intentional? If gather_object is used anywhere in the codebase, it will now call the unpatched version from torch.distributed, which could bypass the ReloadableProcessGroup logic and potentially cause issues. This change seems unrelated to the documentation fixes in the PR description, which is unexpected.
| @@ -3,44 +3,53 @@ | |||
| const STORAGE_KEY = 'vime-doc-lang'; | |||
| // Default language EN has no URL prefix; Chinese uses '/zh/' inserted after optional repo root. | |||
There was a problem hiding this comment.
This comment appears to be outdated given the changes in buildTargetUrl. The new logic seems to add a language prefix for English (e.g., /en/) in most cases, not just for Chinese. To avoid confusion, consider updating this comment to reflect that both languages may have an explicit prefix in the URL.
| if(repoRootLen > 0){ | ||
| parts.splice(repoRootLen, 0, target); | ||
| } else if(target === 'zh'){ | ||
| parts.unshift('zh'); | ||
| } else if(parts[0] !== 'en'){ | ||
| parts.unshift('en'); | ||
| } |
There was a problem hiding this comment.
The logic for inserting the target language code is a bit complex and can be simplified for better readability. Using parts.splice(repoRootLen, 0, target) can achieve the same result more concisely, as splice with an index of 0 is equivalent to unshift.
The condition else if(parts[0] !== 'en') is particularly confusing. After the current language is removed from parts, this condition will almost always be true when switching to English, making it effectively redundant. A single splice call would be clearer.
parts.splice(repoRootLen, 0, target);
|
Title says doc fix, but 4/5 files are not docs. Can we split? |
2 participants
Fix the Sphinx language toggle so it handles Read the Docs project translation URLs.
This updates the toggle logic to work with paths like /projects/vime/en/latest/ and /projects/vime/zh/latest/ instead of only the old GitHub Pages-style paths.