chore: vLLM, kernels, and flashinfer upgrades - #623
Conversation
Updates the requirements on kernels to permit the latest version. --- updated-dependencies: - dependency-name: kernels dependency-version: 0.16.0 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [vllm](https://github.com/vllm-project/vllm) from 0.20.0 to 0.24.0. - [Release notes](https://github.com/vllm-project/vllm/releases) - [Changelog](https://github.com/vllm-project/vllm/blob/main/RELEASE.md) - [Commits](vllm-project/vllm@v0.20.0...v0.24.0) --- updated-dependencies: - dependency-name: vllm dependency-version: 0.24.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [flashinfer-cubin](https://github.com/flashinfer-ai/flashinfer) from 0.6.8.post1 to 0.6.13. - [Release notes](https://github.com/flashinfer-ai/flashinfer/releases) - [Commits](flashinfer-ai/flashinfer@v0.6.8.post1...v0.6.13) --- updated-dependencies: - dependency-name: flashinfer-cubin dependency-version: 0.6.13 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates dependency pins and vLLM source entries, refreshes cu129 wheel index URLs in install instructions, and adds RoPE override handling in ChangesvLLM dependency, install, and runtime configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR upgrades vLLM from 0.20.0 to 0.24.0 and FlashInfer from 0.6.8.post1 to 0.6.13, adapting the vLLM backend to the new API surface and adding proper RoPE context-extension support via
Confidence Score: 5/5Safe to merge — the vLLM 0.24 API migration is complete and internally consistent, the three issues raised in earlier review rounds are all resolved, and the new RoPE override path is covered by targeted tests. The FlashInfer version mismatch flagged in a prior round is now fixed in both the cpu and cu129 extras. The vLLM index name is corrected. The test assertion for hf_overrides now matches the single-key return value of _build_rope_hf_overrides. The TokensPrompt migration, max_model_len alignment, and RopeScaling.rope_parameters round-trip are all covered by new unit tests. No files require special attention beyond the minor transformers pin style difference in pyproject.toml. Important Files Changed
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: mkornfield <mkornfield@nvidia.com>
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
The upgrade hamster wheel never ends.
Is this motivated by a particular issue, or just general bump of our deps to keep up?
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/nemo_safe_synthesizer/generation/vllm_backend.py (1)
312-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why
hf_overridesis needed here.The rationale for conditionally injecting
hf_overrides(vLLM 0.24 requiring explicit RoPE config overrides whenmax_model_lenexceeds the model's native context) only appears in the test docstring, not as a source comment. Since this is a non-obvious, version-specific invariant, a brief inline comment here would help future maintainers understand why this branch exists without needing to trace back to the test file.model_ref = ModelRef.parse(self.config.training.pretrained_model) llm_kwargs: dict[str, Any] = {} # vLLM 0.24 requires explicit rope_parameters hf_overrides when max_model_len # extends beyond the model's native context via rope_scaling. if hf_overrides := _build_rope_hf_overrides(self.model_metadata): llm_kwargs["hf_overrides"] = hf_overridestests/generation/test_vllm_backend.py (1)
398-441: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd boundary coverage for
rope_scaling.factor <= 1.0withrope_scalingset.The two tests cover
rope_scaling is None(no override) andfactor=2.0(override applied), but_build_rope_hf_overrides's explicitfactor <= 1.0branch is never exercised with a non-Nonerope_scaling(e.g.,factor=1.0). This boundary is exactly the condition guarding whetherhf_overridesgets sent to vLLM, so it's worth a focused test.def test_initialize_omits_hf_overrides_when_rope_scaling_factor_not_extended(...): mock_model_metadata.rope_scaling = RopeScaling(rope_type="linear", factor=1.0, theta=10000.0) ... assert "hf_overrides" not in mock_vllm.call_args.kwargs
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1594b711-f9d7-4961-bfe9-a6b58c1e3f1d
📒 Files selected for processing (2)
src/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Greptile Review
- GitHub Check: Typecheck
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported
**/*.py: Source code must remain Python 3.11 syntax-compatible; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Use ruff for formatting and linting via mise run format and mise run check tasks; run formatting before committing
Use ty for type checking via mise run check task; ensure type hints are present and valid
New features must include tests; bug fixes must include regression tests
**/*.py: UseBaseSettingsfor env/CLI settings, preferAliasChoicesfor per-field dual naming, and useenv_prefixonly for simple settings classes with a shared prefix.
UseField(description=...)as the canonical field docstring for Pydantic models, and always include it.
Use assignment-styleField(default=..., description="...")as the default for model fields; prefer it overAnnotatedunless extra metadata is needed.
UseAnnotatedonly when the field carries additional metadata beyondField()(for example validators, reusable constrained aliases, nested-type constraints, or discriminated unions).
WhenAnnotatedis used, place defaults as bare assignments (= value), exceptdefault_factory, which should still use assignment-styleField(default_factory=...).
For immutable value objects and validators, prefer@dataclass(frozen=True); use mutable dataclasses only for builders, accumulators, and pipeline state.
Usefield(default_factory=list)for mutable defaults; never use= [].
UseStrEnumfor string-valued enums used in configs or serialization; use plainEnumfor internal-only named constants.
Useobservability.get_logger(__name__)for logging; do not calllogging.getLogger()orstructlog.get_logger()direc...
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insidesrc/, for examplefrom ..observability import get_logger.
Every directory undersrc/that contains Python files must include an__init__.pyfile.
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file must include the required SPDX copyright and license header; use HTML comments for Markdown, hash comments for.py,.sh,.yaml, and.yml, and hash-comment headers inside YAML frontmatter for Markdown files with frontmatter.
Ensure files end with a newline and have no trailing whitespace; use a single space between sentences.
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.py
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pytests/generation/test_vllm_backend.py
src/nemo_safe_synthesizer/generation/**/*.py
⚙️ CodeRabbit configuration file
Review generation changes for retry loops, stopping conditions, invalid record handling, regex/structured output contracts, backend teardown, memory cleanup, and vLLM assumptions.
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/generation/test_vllm_backend.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/generation/test_vllm_backend.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning Tests
All mise test tasks, grouped by scope:
mise run test # Unit (excludes slow, e2e, and smoke) mise run test:unit-slow # Unit tests including slow (excludes e2e and smoke) mise run test:smoke # CPU smoke tests (~few min, no GPU required) mise run test:smoke:gpu # All staged GPU smoke tests (requires CUDA) mise run test:smoke:gpu:train-only mise run test:smoke:gpu:generation mise run test:smoke:gpu:resume mise run test:smoke:gpu:structured-generation mise run test:smoke:gpu:timeseries mise run test:smoke:gpu:smollm2 mise run test:e2e # All e2e (requires CUDA) -- runs default + dp mise run test:e2e:default # e2e default (no-DP) tests only mise run test:e2e:dp # e2e DP tests only mise run test:ci # CI unit tests with coverage (excludes slow, e2e, gpu, smoke) mise run test:ci-slow # CI slow tests with coverage mise run test:ci-container # CI tests in a Linux container (Docker/Podman)Run a single test:
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/generation/test_vllm_backend.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto gate tests on optional dependencies that require specific extras (e.g.,sentence_transformers,vllm)
Run vLLM tests with separate pytest invocations (one per file) using-n 0(single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruffT201is suppressed fortests/directory) and should...
Files:
tests/generation/test_vllm_backend.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/generation/test_vllm_backend.py
🧠 Learnings (2)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/generation/test_vllm_backend.py
📚 Learning: 2026-06-04T16:14:09.868Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 526
File: tests/generation/test_vllm_backend.py:399-509
Timestamp: 2026-06-04T16:14:09.868Z
Learning: In this repo, only apply `pytest.mark.vllm` to smoke tests under `tests/smoke/` that actually run real vLLM GPU generation and therefore require per-file process isolation (e.g., `test-smoke-gpu-*` Makefile targets). Do not apply `pytest.mark.vllm` to unit-style tests under `tests/generation/` that merely import `vllm_backend` but never instantiate a real vLLM engine and never call `.generate()` (GPU not required). Note that `tests/conftest.py` auto-marks these as `unit` via `pytest_collection_modifyitems`, and `vllm` is not among the auto-mark categories—so if a test in `tests/generation/` has `vllm`, it should be treated as a review issue unless it meets the real GPU generation criteria above.
Applied to files:
tests/generation/test_vllm_backend.py
🔇 Additional comments (2)
tests/generation/test_vllm_backend.py (1)
23-23: LGTM!Also applies to: 46-47
src/nemo_safe_synthesizer/generation/vllm_backend.py (1)
79-94: 🎯 Functional CorrectnessNo issue here:
VllmBackend.initialize()is only reached with fully populated metadata, andModelMetadata.stub()stays on the preflight path. Thebase_max_seq_lengthfallback mismatch does not reach model loading.> Likely an incorrect or invalid review comment.
9e28e7e to
9c4a585
Compare
Signed-off-by: mkornfield <mkornfield@nvidia.com>
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
Slurm testing is successful? Fix up the test expectation and looks good.
Signed-off-by: mkornfield <mkornfield@nvidia.com>
binaryaaron
left a comment
There was a problem hiding this comment.
agent (
review-pr): Request changes for vLLM 0.24 and Transformers 5.12 compatibility gaps
I found several migration gaps that should be addressed before merge.
Required changes
-
[P1] Preserve native Transformers 5 RoPE parameters
RopeScaling.from_autoconfig()still reads the removed top-levelconfig.rope_theta. Transformers 5.12 stores this value inconfig.rope_parameters["rope_theta"].For a standard Qwen configuration, NSS currently converts the native theta from
1_000_000to the fallback10_000. The newhf_overridesthen replaces the correct vLLM value with the fallback. This is reachable through the normal train, save-artifact, resume, and generate flow.The same boundary silently drops scheme-specific fields required by
dynamic,yarn, andllama3. Please preserve and validate the standardized nativerope_parameters, then add a realQwen2Configround-trip regression test. -
[P2] Keep
max_position_embeddingsat the original context lengthvLLM treats
max_position_embeddingsas the pre-scaling context and multiplies it by the RoPE factor. The PR sets it to the already-extended length, so the rotary cache is scaled twice.For
base=2048andfactor=2, the required cache length is4096; the current override produces8192. Keep the top-levelLLM(max_model_len=...)argument and removemax_position_embeddingsand the redundant nestedmax_model_lenfromhf_overrides. -
[P2] Update the removed token-ID generation API
vLLM 0.24 no longer accepts
LLM.generate(prompt_token_ids=...). The_generate(input_ids=...)branches now raiseTypeError. Pass token IDs throughprompts=and add flat and batched token-ID coverage using the real 0.24 signature. -
[P2] Update the dependency troubleshooting documentation
docs/user-guide/troubleshooting.mdstill recommends Transformers>=5.6,<6with vLLM0.20.0. Please update this to the supported Transformers 5.12 and vLLM 0.24 pairing.
Follow-up cleanup
- Remove or replace the obsolete
num_beams -> beam_widthmapping.SamplingParams0.24 no longer acceptsbeam_width. - Pass
hf_overrides=directly instead of routing it throughdict[str, Any]and**llm_kwargs. - Use the public vLLM
AttentionConfigto validate the configured attention backend. - Replace the
TokenizerLikecast with an encode-only localProtocol.
Validation performed
- Focused generation tests passed.
- Type checks, lint, lock validation, and dependency resolution passed.
- Transformers and vLLM runtime probes reproduced the issues above.
- Existing GPU smoke coverage does not exercise scaled-context generation.
StructuredOutputsConfig, top-level max_model_len, and the current shutdown strategy remain compatible with vLLM 0.24.
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Summary
Pre-Review Checklist
Ensure that the following pass:
mise run format && mise run checkor via prek validation.mise run testpasses locallymise run test:e2epasses locallymise run test:ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes
Summary by CodeRabbit
max_model_lenand RoPE-related overrides.