Kimik26 humming#501
Open
gaogaoSpark wants to merge 16 commits into
Open
Conversation
Signed-off-by: Zheng Luo <zheluo@nvidia.com>
…i-dynamo#392) (ai-dynamo#398) Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…ted (ai-dynamo#400) Signed-off-by: Hyunjae Woo <hwoo@nvidia.com>
…i-dynamo#401) Signed-off-by: Hyunjae Woo <hwoo@nvidia.com>
Signed-off-by: Zheng Luo <zheluo@nvidia.com>
…combos (backport to 0.4.0) (ai-dynamo#405) Signed-off-by: Zhongdongming Dai <zhongdongmin@nvidia.com>
Signed-off-by: Zheng Luo <zheluo@nvidia.com>
…port to 0.4.0) (ai-dynamo#409) Signed-off-by: Zhongdongming Dai <zhongdongmin@nvidia.com>
… false cache hits (backport to 0.4.0) (ai-dynamo#412) Signed-off-by: Hyunjae Woo <hwoo@nvidia.com>
…(0.4.0 QA) (ai-dynamo#420) Signed-off-by: Hyunjae Woo <hwoo@nvidia.com>
ai-dynamo#426) Signed-off-by: Hyunjae Woo <hwoo@nvidia.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughChangesThe release updates build and deployment workflows, standardizes ModelExpress loader and address configuration, adds manifest-aware tensor and quantization handling, revises RDMA retry behavior, introduces provider-scoped registry migration, and adds model deletion plus stale-cache recovery. Release tooling and deployment guidance
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelexpress_server/src/p2p/state.rs (1)
218-242: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert that the new tensor metadata survives conversion.
test_tensor_record_conversionpopulates the new layout fields but only verifiesnameandaddr. The two worker round-trip tests use default metadata and never compareback.tensors, so a conversion that drops shape, stride, storage, or original fields would still pass. Add non-default values and assert the complete tensor metadata round-trip.Also applies to: 248-277, 283-315
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_server/src/p2p/state.rs` around lines 218 - 242, Expand test_tensor_record_conversion and the two worker round-trip tests to use non-default tensor metadata, including shape, stride, storage, layout, original shape, original dtype, and byte-size fields. After converting through TensorRecord and back, assert the complete tensor descriptor metadata, including back.tensors in the worker tests, while preserving the existing name and address checks.
🧹 Nitpick comments (8)
modelexpress_common/src/providers/ngc.rs (1)
1462-1466: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the mocks enforce the Authorization contract.
These mocks only match method/path. They still pass if bearer credentials leak to presigned
/dl/*URLs or are omitted from fallback/files/*requests. RejectAuthorizationon presigned requests and require the expected bearer header on checksum/fallback requests.Also applies to: 1595-1609, 1649-1652
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_common/src/providers/ngc.rs` around lines 1462 - 1466, Update the WireMock setups around the presigned `/dl/*` mocks and the checksum/fallback `/files/*` mocks to enforce headers: reject requests containing Authorization for presigned downloads, and require the expected bearer Authorization header on checksum/fallback requests. Apply the same matcher changes to the additional mock blocks noted in the comment while preserving their existing method, path, and response behavior.modelexpress_client/python/modelexpress/quantization_providers/humming.py (1)
183-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated "record runtime tensor" logic.
_record_runtime_tensor,_record_runtime_tensor_value, and_record_unique_packed_weighteach repeat the same "get-or-createHUMMING_RUNTIME_TENSORS_ATTRdict, store tensor, stampmx_runtime_role/mx_replace_policy" block. Extracting a shared helper (e.g._store_runtime_tensor(module, leaf, tensor)) would remove ~15 duplicated lines and reduce the risk of the three copies drifting apart.♻️ Proposed helper extraction
+def _store_runtime_tensor(module: nn.Module, leaf: str, tensor: torch.Tensor) -> None: + runtime_tensors = getattr(module, HUMMING_RUNTIME_TENSORS_ATTR, None) + if not isinstance(runtime_tensors, dict): + runtime_tensors = {} + setattr(module, HUMMING_RUNTIME_TENSORS_ATTR, runtime_tensors) + runtime_tensors[leaf] = tensor + tensor.mx_runtime_role = HUMMING_RUNTIME_ROLE + tensor.mx_replace_policy = NO_STRUCTURAL_REPLACE_POLICYThen replace the duplicated blocks in
_record_runtime_tensor,_record_runtime_tensor_value, and_record_unique_packed_weightwith calls to_store_runtime_tensor(layer, str(name), tensor).Also applies to: 303-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_client/python/modelexpress/quantization_providers/humming.py` around lines 183 - 229, Extract the shared runtime-tensor storage logic into a helper such as _store_runtime_tensor(module, leaf, tensor), including creation or reuse of HUMMING_RUNTIME_TENSORS_ATTR and application of both metadata attributes. Update _record_runtime_tensor, _record_runtime_tensor_value, and _record_unique_packed_weight to validate and prepare their tensors as before, then delegate storage to the helper using the stringified name, without changing their existing filtering behavior.modelexpress_client/python/tests/test_vllm_loader.py (1)
1070-1103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert that transfer failure performs rollback before aborting.
This test still passes if the critical
rollback(ctx)call is removed.Proposed test improvement
strategy._load_as_target = fake_load_as_target +strategy.rollback = MagicMock() ... assert exc.value.mutated is True assert attempts == ["w-1"] +strategy.rollback.assert_called_once_with(ctx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_client/python/tests/test_vllm_loader.py` around lines 1070 - 1103, Update test_source_transfer_error_aborts_rdma_as_mutated to track or mock the strategy rollback operation and assert rollback(ctx) is called before the StrategyFailed exception is propagated. Keep the existing assertions that the error is mutated and no second candidate is attempted, while making the test fail if rollback is removed.modelexpress_server/src/p2p/k8s_types.rs (1)
344-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOwnership/quantization metadata fields aren't verified by either roundtrip test. Both
TensorDescriptorJson(k8s ConfigMap schema) andTensorRecordJson(Redis schema) addedtensor_kind,owner_module,owner_class,quant_method,runtime_role,replace_policy, but neither roundtrip test asserts these fields survive serialization — the shared root cause is that new-field coverage wasn't extended to assertions when the structs grew.
modelexpress_server/src/p2p/k8s_types.rs#L344-L355: addassert_eq!calls fortensor_kind,owner_module,owner_class,quant_method,runtime_role,replace_policyagainst theoriginalvalues already set at lines 333-338 (e.g.quant_method: "humming").modelexpress_server/src/p2p/backend/redis.rs#L671-L682: set these same fields to non-default values in therecordfixture (lines 645-662) and add matchingassert_eq!checks after the roundtrip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_server/src/p2p/k8s_types.rs` around lines 344 - 355, The roundtrip tests omit assertions for the new ownership and quantization metadata. In modelexpress_server/src/p2p/k8s_types.rs lines 344-355, extend the existing TensorDescriptorJson roundtrip assertions with tensor_kind, owner_module, owner_class, quant_method, runtime_role, and replace_policy comparisons against original. In modelexpress_server/src/p2p/backend/redis.rs lines 671-682, populate those same fields with non-default values in the record fixture and assert each survives serialization and deserialization.docker/Dockerfile.client-wheel (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
uvfor reproducible release artifacts.The image and
auditwheelare pinned, butpip install --upgrade uvresolves a different build frontend over time. Pinuvto an approved exact version (and preferably verify its hash) so wheel metadata and build behavior remain reproducible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/Dockerfile.client-wheel` at line 37, Update the pip install command in the Dockerfile to pin uv to the approved exact version, keeping auditwheel==6.7.0 unchanged; add a hash verification for uv if the project’s dependency-pinning conventions support it.modelexpress_client/python/modelexpress/engines/vllm/adapter.py (2)
262-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuantization-string derivation duplicated five times.
str(getattr(self.model_config, "quantization", "") or "")is recomputed independently indiscover_tensors,apply_weight_iter,load_via_native,_process_weights_after_loading, and_quantization_provider. Consider a small cached property (e.g.self._quantization_str) that both_quantization_provider()andprovider.enabled(...)call sites reuse.Also applies to: 72-81, 133-137, 171-176, 226-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_client/python/modelexpress/engines/vllm/adapter.py` around lines 262 - 265, The quantization string derivation is duplicated across discover_tensors, apply_weight_iter, load_via_native, _process_weights_after_loading, and _quantization_provider. Add a cached instance value such as _quantization_str, initialize it from model_config.quantization once, and update all provider.enabled(...) call sites plus _quantization_provider() to reuse it.
410-441: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAttention-refresh gate matches exact class names; consider a base-class check.
type(module).__name__ not in {"Attention", "MLAAttention", "MMEncoderAttention"}is accurate for current vLLM (confirmed these are real class names), but it's brittle: any attention-like module vLLM adds or renames in a future version that isn't one of these three literal names would silently skip this refresh — reintroducing the exact profile/warmup failure this hook exists to fix, just for a different module type. vLLM'sAttentionandMLAAttentionboth derive from a sharedvllm.model_executor.layers.attention_layer_base.AttentionLayerBasemarker; anisinstance(module, AttentionLayerBase)check (combined with the existinghasattr(module, "process_weights_after_loading")gate you already have) would be more resilient than name-matching.♻️ Suggested direction
- attention_class_names = { - "Attention", - "MLAAttention", - "MMEncoderAttention", - } + from vllm.model_executor.layers.attention_layer_base import AttentionLayerBase ... for _, module in model.named_modules(): - if type(module).__name__ not in attention_class_names: + if not isinstance(module, AttentionLayerBase): continuePlease verify
MMEncoderAttentionalso derives fromAttentionLayerBasein vLLM 0.23.0 before switching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_client/python/modelexpress/engines/vllm/adapter.py` around lines 410 - 441, Update _refresh_vllm_attention_runtime_tensors to identify attention modules via vLLM’s AttentionLayerBase isinstance check while retaining the existing process_weights_after_loading gate. Verify MMEncoderAttention inherits from AttentionLayerBase in vLLM 0.23.0; preserve explicit handling if it does not, so all currently supported attention modules continue to be refreshed.modelexpress_client/python/modelexpress/nixl_transfer.py (1)
516-528: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winManifest-count warning can miss actual name mismatches.
The warning only fires when
len(source_names) != len(local_names); if counts coincidentally match but the underlying tensor names differ (e.g., all source-only vs. all local-only), no warning is emitted even thoughcommon_nameswould be empty. Sincecommon_namesis already computed, comparing it against both sets gives a strictly better diagnostic for the exact "manifest divergence" scenario this PR targets.♻️ Proposed fix to also detect same-count-different-names mismatches
- if len(source_names) != len(local_names): + if len(common_names) != len(source_names) or len(common_names) != len(local_names): logger.warning( "Tensor manifest count differs before RDMA: source=%d local=%d " "common=%d source_only=%d local_only=%d",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_client/python/modelexpress/nixl_transfer.py` around lines 516 - 528, Update the manifest-divergence condition in the warning block around source_names, local_names, and common_names so it also triggers when the common-name set is smaller than either manifest, not only when their counts differ. Preserve the existing diagnostic fields and warning behavior while covering same-count, different-name manifests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ARCHITECTURE.md`:
- Line 593: Update the later RDMA coordination-flow documentation around the
SourceTransferError handling to state that transfer-time SourceTransferError and
ManifestMismatchError abort RDMA and fall through to the next strategy, rather
than retrying the next candidate; apply the same correction to the corresponding
duplicated entry.
In `@docs/CLI.md`:
- Line 75: Update all three changed health examples in CLI.md, including the
entries around the visible Version value and the referenced later occurrences,
from 0.4.0 to the released 0.4.1 version.
In `@docs/DEPLOYMENT.md`:
- Around line 236-262: Close the bash code fence immediately after the Docker
commands in the “Without buildx” section, before the “CI uploads to Artifactory”
heading, so the heading, prose, and destination-layout table render as Markdown.
In `@examples/p2p_transfer_k8s/client/trtllm/mx-infra-decode.yaml`:
- Line 80: Update the container image tag in the Kubernetes deployment’s image
configuration from modelexpress-server:0.4.0 to modelexpress-server:0.4.1,
leaving the surrounding deployment settings unchanged.
In `@examples/p2p_transfer_k8s/client/vllm/aws_efa/README.md`:
- Around line 48-50: Align the AWS EFA example’s documentation and Kubernetes
resources: in examples/p2p_transfer_k8s/client/vllm/aws_efa/README.md lines
48-50, document the instance-specific GPU requirement; in
examples/p2p_transfer_k8s/client/vllm/aws_efa/vllm-aws-efa.yaml lines 117-133,
replace the hard-coded 8-GPU and tensor-parallel settings with values compatible
with the documented p6e instance, or constrain the example to an 8-GPU p5 node.
In
`@examples/p2p_transfer_k8s/server/kubernetes_backend/modelexpress-server-kubernetes.yaml`:
- Line 44: Update the Model Express image tag from 0.4.0 to 0.4.1 in both
examples/p2p_transfer_k8s/server/kubernetes_backend/modelexpress-server-kubernetes.yaml:44-44
and
examples/p2p_transfer_k8s/server/redis_backend/modelexpress-server-redis.yaml:78-78,
preserving the existing image repositories and manifest structure.
In `@helm/README.md`:
- Line 104: Complete the image-version bump to 0.4.1: update helm/README.md
lines 104 and 298, helm/test-values.yaml line 10, and
helm/values-development.yaml line 10 so all documented, test, and development
image references use 0.4.1 instead of 0.4.0.
In `@helm/values.yaml`:
- Around line 10-13: Update image.tag from 0.4.0 to 0.4.1 in helm/values.yaml
lines 10-13 and helm/values-production.yaml lines 7-10, keeping the repository
and pullPolicy unchanged.
In `@modelexpress_client/python/modelexpress/load_strategy/rdma_strategy.py`:
- Around line 131-139: Update the RDMA retry handling in
_has_cause/StrategyFailed processing so manifest validation is transactional:
before continuing to another source after ManifestMismatchError, roll back or
reinitialize the target to a clean state rather than setting target_prepared as
if it were reusable. In
modelexpress_client/python/modelexpress/load_strategy/rdma_strategy.py lines
131-139, implement the cleanup before continue; in
modelexpress_client/python/tests/test_vllm_loader.py lines 1032-1068, replace
assert_not_called() with assertions that verify the target is clean before the
second candidate is attempted.
In `@modelexpress_client/python/modelexpress/nixl_transfer.py`:
- Around line 231-234: Extract the duplicated layout-kind selection into a
shared helper and use it in both _build_tensor_descriptors and
receive_from_source. The helper should return "storage_view" for names ending in
".__storage" and "contiguous" otherwise, so sender descriptor construction and
receiver validation remain consistent.
In `@modelexpress_client/python/tests/test_vllm_adapter.py`:
- Around line 548-592: Update
test_prepare_rdma_target_from_manifest_keeps_matching_humming_packed_tensor so
its TensorDescriptor represents a Humming packed tensor, using the Humming
quantization method, packed-tensor runtime role, and matching replacement
policy/layout metadata. Preserve the assertions that the model, weight, and
quant_method remain unchanged, and add a separate test covering quant-method
realignment for a valid unquantized manifest with the same layout.
In `@modelexpress_client/src/bin/modules/handlers.rs`:
- Around line 561-573: Update the cache enumeration in the handler around
storage_config.clear_all so get_cache_stats failures are not converted to an
empty list. Propagate the enumeration error before clearing, or explicitly
report that registry cleanup was skipped; do not proceed with clear_all while
silently treating a failure as zero cached models.
In `@modelexpress_common/src/providers/ngc.rs`:
- Around line 400-401: Update the serde_json parsing error context in the
ArtifactFilesResponse parsing flow to avoid including the raw body, which
contains presigned URLs; retain a descriptive parse-failure message without
exposing manifest contents.
In `@modelexpress_server/src/registry/backend/kubernetes.rs`:
- Around line 137-145: The provider-scoped lookup currently returns the first
matching CR/key regardless of provider; update find_existing_cr in
modelexpress_server/src/registry/backend/kubernetes.rs (lines 137-145) to accept
and use the requested provider, update the corresponding Redis lookup in
modelexpress_server/src/registry/backend/redis.rs (lines 244-273) to query that
provider’s key rather than the first non-empty candidate, and update the
download-status polling call in modelexpress_server/src/services.rs (lines
828-835) to pass the active provider.
- Around line 176-187: Update the successful create branch in the
`self.api().create` flow to roll back the newly created scoped CR when
`patch_status` fails: capture the patch error, delete the created resource using
the existing Kubernetes API and scoped resource identity, then propagate the
original failure. Preserve the current status migration and success behavior
when `patch_status` succeeds.
In `@modelexpress_server/src/services.rs`:
- Around line 540-546: The model deletion RPC currently ignores failures from
tracker.delete_status, so update the deletion flow to propagate that failure as
an appropriate gRPC error and only report success after deletion completes. In
modelexpress_server/src/services.rs:540-546, handle the result of
tracker.delete_status; in modelexpress_server/src/services.rs:749-750, ensure
stale recovery retries claiming only after deletion succeeds.
- Around line 518-540: The provider-bearing delete_model RPC currently deletes
all records for a model name; propagate provider through delete_status and the
registry manager so deletion is provider-scoped. In
modelexpress_server/src/services.rs:518-540 update delete_model’s call chain to
pass provider; in modelexpress_server/src/registry/backend/kubernetes.rs:426-439
and modelexpress_server/src/registry/backend/redis.rs:322-326 delete only the
provider-scoped record plus any matching-provider legacy record; in
workspace-tests/tests/registry_backend_redis.rs:423-426 assert the unselected
provider record remains.
---
Outside diff comments:
In `@modelexpress_server/src/p2p/state.rs`:
- Around line 218-242: Expand test_tensor_record_conversion and the two worker
round-trip tests to use non-default tensor metadata, including shape, stride,
storage, layout, original shape, original dtype, and byte-size fields. After
converting through TensorRecord and back, assert the complete tensor descriptor
metadata, including back.tensors in the worker tests, while preserving the
existing name and address checks.
---
Nitpick comments:
In `@docker/Dockerfile.client-wheel`:
- Line 37: Update the pip install command in the Dockerfile to pin uv to the
approved exact version, keeping auditwheel==6.7.0 unchanged; add a hash
verification for uv if the project’s dependency-pinning conventions support it.
In `@modelexpress_client/python/modelexpress/engines/vllm/adapter.py`:
- Around line 262-265: The quantization string derivation is duplicated across
discover_tensors, apply_weight_iter, load_via_native,
_process_weights_after_loading, and _quantization_provider. Add a cached
instance value such as _quantization_str, initialize it from
model_config.quantization once, and update all provider.enabled(...) call sites
plus _quantization_provider() to reuse it.
- Around line 410-441: Update _refresh_vllm_attention_runtime_tensors to
identify attention modules via vLLM’s AttentionLayerBase isinstance check while
retaining the existing process_weights_after_loading gate. Verify
MMEncoderAttention inherits from AttentionLayerBase in vLLM 0.23.0; preserve
explicit handling if it does not, so all currently supported attention modules
continue to be refreshed.
In `@modelexpress_client/python/modelexpress/nixl_transfer.py`:
- Around line 516-528: Update the manifest-divergence condition in the warning
block around source_names, local_names, and common_names so it also triggers
when the common-name set is smaller than either manifest, not only when their
counts differ. Preserve the existing diagnostic fields and warning behavior
while covering same-count, different-name manifests.
In `@modelexpress_client/python/modelexpress/quantization_providers/humming.py`:
- Around line 183-229: Extract the shared runtime-tensor storage logic into a
helper such as _store_runtime_tensor(module, leaf, tensor), including creation
or reuse of HUMMING_RUNTIME_TENSORS_ATTR and application of both metadata
attributes. Update _record_runtime_tensor, _record_runtime_tensor_value, and
_record_unique_packed_weight to validate and prepare their tensors as before,
then delegate storage to the helper using the stringified name, without changing
their existing filtering behavior.
In `@modelexpress_client/python/tests/test_vllm_loader.py`:
- Around line 1070-1103: Update
test_source_transfer_error_aborts_rdma_as_mutated to track or mock the strategy
rollback operation and assert rollback(ctx) is called before the StrategyFailed
exception is propagated. Keep the existing assertions that the error is mutated
and no second candidate is attempted, while making the test fail if rollback is
removed.
In `@modelexpress_common/src/providers/ngc.rs`:
- Around line 1462-1466: Update the WireMock setups around the presigned `/dl/*`
mocks and the checksum/fallback `/files/*` mocks to enforce headers: reject
requests containing Authorization for presigned downloads, and require the
expected bearer Authorization header on checksum/fallback requests. Apply the
same matcher changes to the additional mock blocks noted in the comment while
preserving their existing method, path, and response behavior.
In `@modelexpress_server/src/p2p/k8s_types.rs`:
- Around line 344-355: The roundtrip tests omit assertions for the new ownership
and quantization metadata. In modelexpress_server/src/p2p/k8s_types.rs lines
344-355, extend the existing TensorDescriptorJson roundtrip assertions with
tensor_kind, owner_module, owner_class, quant_method, runtime_role, and
replace_policy comparisons against original. In
modelexpress_server/src/p2p/backend/redis.rs lines 671-682, populate those same
fields with non-default values in the record fixture and assert each survives
serialization and deserialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0bcd2742-5a07-4d12-8116-c48938152265
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockmodelexpress_client/go/gen/modelexpress/model/model.pb.gois excluded by!**/*.pb.go,!**/gen/**modelexpress_client/go/gen/modelexpress/model/model_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**modelexpress_client/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (93)
.github/actions/check-vcluster-exists/action.yml.github/actions/install-vcluster-cli/action.yml.github/actions/setup-mx-vcluster/action.yml.github/workflows/build-wheels.yml.github/workflows/ci.ymlATTRIBUTIONS_Rust.mdCONTRIBUTING.mdCargo.tomlNOTICESREADME.mdci/k8s/client/trt-llm/Dockerfiledocker/Dockerfiledocker/Dockerfile.client-wheeldocker/docker-compose.ymldocs/ARCHITECTURE.mddocs/CLI.mddocs/DEPLOYMENT.mddocs/SGLANG.mddocs/metadata.mdexamples/dynamo_model_cache_k8s/README.mdexamples/dynamo_model_cache_k8s/agg.yamlexamples/dynamo_p2p_transfer_k8s/README.mdexamples/dynamo_p2p_transfer_k8s/vllm/vllm-multi-node-aggregated.yamlexamples/dynamo_p2p_transfer_k8s/vllm/vllm-single-node-disaggregated.yamlexamples/k8s_service_sources/sources-tp2-single-pod.yamlexamples/k8s_service_sources/sources-tp2.yamlexamples/k8s_service_sources/target.yamlexamples/model_streamer_k8s/README.mdexamples/model_streamer_k8s/client/vllm/README.mdexamples/model_streamer_k8s/client/vllm/vllm-single-node-streamer-azure.yamlexamples/model_streamer_k8s/client/vllm/vllm-single-node-streamer-local.yamlexamples/model_streamer_k8s/client/vllm/vllm-single-node-streamer-s3.yamlexamples/p2p_transfer_k8s/README.mdexamples/p2p_transfer_k8s/client/README.mdexamples/p2p_transfer_k8s/client/sglang/sglang-single-node-p2p.yamlexamples/p2p_transfer_k8s/client/trtllm/mx-infra-decode.yamlexamples/p2p_transfer_k8s/client/vllm/aws_efa/README.mdexamples/p2p_transfer_k8s/client/vllm/aws_efa/vllm-aws-efa.yamlexamples/p2p_transfer_k8s/client/vllm/vllm-multi-node.yamlexamples/p2p_transfer_k8s/client/vllm/vllm-single-node-p2p.yamlexamples/p2p_transfer_k8s/client/vllm/vllm-single-node.yamlexamples/p2p_transfer_k8s/server/kubernetes_backend/modelexpress-server-kubernetes.yamlexamples/p2p_transfer_k8s/server/redis_backend/modelexpress-server-redis.yamlhelm/Chart.yamlhelm/README.mdhelm/test-values.yamlhelm/values-development.yamlhelm/values-production.yamlhelm/values.yamlmodelexpress_client/python/README.mdmodelexpress_client/python/modelexpress/adapter.pymodelexpress_client/python/modelexpress/engines/vllm/adapter.pymodelexpress_client/python/modelexpress/load_strategy/__init__.pymodelexpress_client/python/modelexpress/load_strategy/rdma_strategy.pymodelexpress_client/python/modelexpress/metadata/publish.pymodelexpress_client/python/modelexpress/nixl_transfer.pymodelexpress_client/python/modelexpress/p2p_pb2.pymodelexpress_client/python/modelexpress/quantization_providers/__init__.pymodelexpress_client/python/modelexpress/quantization_providers/base.pymodelexpress_client/python/modelexpress/quantization_providers/humming.pymodelexpress_client/python/modelexpress/quantization_providers/registry.pymodelexpress_client/python/modelexpress/tensor_utils.pymodelexpress_client/python/modelexpress/transfer_safety.pymodelexpress_client/python/modelexpress/types.pymodelexpress_client/python/modelexpress/vllm_worker.pymodelexpress_client/python/pyproject.tomlmodelexpress_client/python/setup.pymodelexpress_client/python/tests/test_k8s_service_client.pymodelexpress_client/python/tests/test_pool_registration.pymodelexpress_client/python/tests/test_rdma_strategy.pymodelexpress_client/python/tests/test_source_id.pymodelexpress_client/python/tests/test_tensor_utils.pymodelexpress_client/python/tests/test_transfer_safety.pymodelexpress_client/python/tests/test_vllm_adapter.pymodelexpress_client/python/tests/test_vllm_loader.pymodelexpress_client/src/bin/modules/handlers.rsmodelexpress_client/src/lib.rsmodelexpress_common/proto/model.protomodelexpress_common/proto/p2p.protomodelexpress_common/src/providers/ngc.rsmodelexpress_server/src/p2p/backend.rsmodelexpress_server/src/p2p/backend/kubernetes.rsmodelexpress_server/src/p2p/backend/redis.rsmodelexpress_server/src/p2p/k8s_types.rsmodelexpress_server/src/p2p/service.rsmodelexpress_server/src/p2p/source_identity.rsmodelexpress_server/src/p2p/state.rsmodelexpress_server/src/registry/backend/kubernetes.rsmodelexpress_server/src/registry/backend/redis.rsmodelexpress_server/src/services.rstest_grpc_transfer_k8s.shtest_multinode_k8s.shworkspace-tests/tests/registry_backend_redis.rs
| | Priority | Strategy | `is_available()` | Behavior | | ||
| |---|---|---|---| | ||
| | p0 | `RdmaStrategy` | NIXL available | `ListSources(READY)`, filter by `worker_rank`, shuffle, try candidates (max 3). On `SourceTransferError` or `ManifestMismatchError`, try next. | | ||
| | p0 | `RdmaStrategy` | NIXL available | `ListSources(READY)`, filter by `worker_rank`, shuffle, try candidates (max 3). Candidate retry covers pre-transfer metadata misses only (no metadata found, fetch error). Once the RDMA receive starts, a `SourceTransferError` or `ManifestMismatchError` aborts RDMA and the chain falls through to the next strategy (GDS, then disk), not the next source. | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the later coordination-flow documentation with the new RDMA retry semantics.
These lines say transfer failures abort RDMA and fall through to the next strategy, but Line 725 still says SourceTransferError retries the next candidate. Update Line 725 so operators do not expect candidate retries after transfer begins.
Also applies to: 606-606
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ARCHITECTURE.md` at line 593, Update the later RDMA coordination-flow
documentation around the SourceTransferError handling to state that
transfer-time SourceTransferError and ManifestMismatchError abort RDMA and fall
through to the next strategy, rather than retrying the next candidate; apply the
same correction to the corresponding duplicated entry.
| Server Health Status | ||
| Status: ok | ||
| Version: 0.1.0 | ||
| Version: 0.4.0 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the examples to the released 0.4.1 version.
The PR bumps the release from 0.4.0 to 0.4.1, but all three changed health examples still display 0.4.0.
Proposed fix
- Version: 0.4.0
+ Version: 0.4.1
-# Output: {"version":"0.4.0","status":"ok","uptime":120}
+# Output: {"version":"0.4.1","status":"ok","uptime":120}
-# "version": "0.4.0",
+# "version": "0.4.1",Also applies to: 203-203, 212-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/CLI.md` at line 75, Update all three changed health examples in CLI.md,
including the entries around the visible Version value and the referenced later
occurrences, from 0.4.0 to the released 0.4.1 version.
Comment on lines
+236
to
262
| Without buildx (single arch, matches the host): | ||
|
|
||
| ```bash | ||
| docker build -f docker/Dockerfile.client-wheel --target builder -t mx-wheel-builder . | ||
| docker run --rm -v "$PWD/dist:/out" mx-wheel-builder bash -lc 'cp -r /dist/. /out/' | ||
|
|
||
| #### CI uploads to Artifactory | ||
|
|
||
| `.github/workflows/build-wheels.yml` runs this Dockerfile on every PR | ||
| (via `copy-pr-bot` mirroring into `pull-request/<pr_id>` branches) and | ||
| every push to `main` / `release/**`, building both archs in parallel on | ||
| velonix self-hosted runners and uploading the artifacts to NV Artifactory. | ||
|
|
||
| Destination layout under `${ARTIFACTORY_PYPI_REPO_NAME}`: | ||
|
|
||
| | Event | Subpath | | ||
| |---|---| | ||
| | `push` to `pull-request/<pr_id>` (copy-pr-bot mirror) | `pr/<pr_id>/<commit_sha>/<run_id>/<run_attempt>/<arch>/` | | ||
| | `push` to `main`, `release/**` | `post-merge/<commit_sha>/<run_id>/<run_attempt>/<arch>/` | | ||
|
|
||
| Each path contains the 6 artifacts from one arch: 4 manylinux wheels | ||
| (cp310-cp313), 1 `py3-none-any` wheel, and 1 sdist. The upload step is | ||
| gated on the `automated-release` GitHub environment, which holds three | ||
| secrets: `ARTIFACTORY_URL`, `ARTIFACTORY_TOKEN` (JFrog identity token), | ||
| and `ARTIFACTORY_PYPI_REPO_NAME`. | ||
|
|
||
| ### Custom Client Image (P2P Transfers) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unclosed code fence swallows the following section.
The ```bash block opened before the "Without buildx" example (line 238) is never closed. There is no ``` before #### CI uploads to Artifactory (line 242), so that heading, the prose, and the destination-layout table (through line 260) will all render as literal text inside the code block.
📝 Proposed fix
docker build -f docker/Dockerfile.client-wheel --target builder -t mx-wheel-builder .
docker run --rm -v "$PWD/dist:/out" mx-wheel-builder bash -lc 'cp -r /dist/. /out/'
+```
#### CI uploads to Artifactory🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/DEPLOYMENT.md` around lines 236 - 262, Close the bash code fence
immediately after the Docker commands in the “Without buildx” section, before
the “CI uploads to Artifactory” heading, so the heading, prose, and
destination-layout table render as Markdown.
| containers: | ||
| - name: server | ||
| image: nvcr.io/nvidia/ai-dynamo/modelexpress-server:0.3.0 | ||
| image: nvcr.io/nvidia/ai-dynamo/modelexpress-server:0.4.0 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the 0.4.1 server image.
This example still deploys modelexpress-server:0.4.0, while this PR targets Model Express 0.4.1. Update the image tag so the example exercises the released server version and stays compatible with the accompanying client changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/p2p_transfer_k8s/client/trtllm/mx-infra-decode.yaml` at line 80,
Update the container image tag in the Kubernetes deployment’s image
configuration from modelexpress-server:0.4.0 to modelexpress-server:0.4.1,
leaving the surrounding deployment settings unchanged.
Comment on lines
+48
to
+50
| The manifest defaults to `deepseek-ai/DeepSeek-V3` with | ||
| `--tensor-parallel-size 8` and eight GPUs. If you switch models, update the | ||
| tensor-parallel size and GPU request/limit to match that model. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the AWS EFA documentation and resource requests.
The README advertises a 4-GPU p6e instance, but the manifest hard-codes 8 GPUs. Either constrain this example to 8-GPU p5 nodes or provide instance-specific GPU and tensor-parallel settings.
examples/p2p_transfer_k8s/client/vllm/aws_efa/README.md#L48-L50: document the instance-specific GPU requirement.examples/p2p_transfer_k8s/client/vllm/aws_efa/vllm-aws-efa.yaml#L117-L133: avoid hard-coding an unschedulable 8-GPU configuration for p6e.
📍 Affects 2 files
examples/p2p_transfer_k8s/client/vllm/aws_efa/README.md#L48-L50(this comment)examples/p2p_transfer_k8s/client/vllm/aws_efa/vllm-aws-efa.yaml#L117-L133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/p2p_transfer_k8s/client/vllm/aws_efa/README.md` around lines 48 -
50, Align the AWS EFA example’s documentation and Kubernetes resources: in
examples/p2p_transfer_k8s/client/vllm/aws_efa/README.md lines 48-50, document
the instance-specific GPU requirement; in
examples/p2p_transfer_k8s/client/vllm/aws_efa/vllm-aws-efa.yaml lines 117-133,
replace the hard-coded 8-GPU and tensor-parallel settings with values compatible
with the documented p6e instance, or constrain the example to an 8-GPU p5 node.
Comment on lines
400
to
+401
| let parsed: ArtifactFilesResponse = serde_json::from_str(&body) | ||
| .with_context(|| format!("Failed to parse NGC team files response: {body}"))?; | ||
| .with_context(|| format!("Failed to parse NGC files manifest response: {body}"))?; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not expose presigned URLs in parse errors.
The raw manifest body contains signature-bearing URLs. Including it in the error context can leak credentials through logs or API responses.
- let parsed: ArtifactFilesResponse = serde_json::from_str(&body)
- .with_context(|| format!("Failed to parse NGC files manifest response: {body}"))?;
+ let parsed: ArtifactFilesResponse = serde_json::from_str(&body)
+ .context("Failed to parse NGC files manifest response")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let parsed: ArtifactFilesResponse = serde_json::from_str(&body) | |
| .with_context(|| format!("Failed to parse NGC team files response: {body}"))?; | |
| .with_context(|| format!("Failed to parse NGC files manifest response: {body}"))?; | |
| let parsed: ArtifactFilesResponse = serde_json::from_str(&body) | |
| .context("Failed to parse NGC files manifest response")?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelexpress_common/src/providers/ngc.rs` around lines 400 - 401, Update the
serde_json parsing error context in the ArtifactFilesResponse parsing flow to
avoid including the raw body, which contains presigned URLs; retain a
descriptive parse-failure message without exposing manifest contents.
Comment on lines
+137
to
+145
| /// First existing CR among the candidates (provider-scoped keys, then legacy). Used by | ||
| /// name-addressed reads; in practice a name maps to a single provider. | ||
| async fn find_existing_cr(&self, model_name: &str) -> RegistryResult<Option<ModelCacheEntry>> { | ||
| for cr_name in Self::candidate_cr_names(model_name) { | ||
| if let Some(cr) = self.get_cr(&cr_name).await? { | ||
| return Ok(Some(cr)); | ||
| } | ||
| } | ||
| Ok(None) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Provider-scoped storage still has provider-agnostic reads. Because multiple providers may now hold the same model name, selecting the first candidate can return an unrelated status and prematurely complete downloads.
modelexpress_server/src/registry/backend/kubernetes.rs#L137-L145: accept a provider instead of returning the first scoped CR.modelexpress_server/src/registry/backend/redis.rs#L244-L273: query the requested provider key rather than the first non-empty candidate.modelexpress_server/src/services.rs#L828-L835: pass the active provider while polling download status.
📍 Affects 3 files
modelexpress_server/src/registry/backend/kubernetes.rs#L137-L145(this comment)modelexpress_server/src/registry/backend/redis.rs#L244-L273modelexpress_server/src/services.rs#L828-L835
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelexpress_server/src/registry/backend/kubernetes.rs` around lines 137 -
145, The provider-scoped lookup currently returns the first matching CR/key
regardless of provider; update find_existing_cr in
modelexpress_server/src/registry/backend/kubernetes.rs (lines 137-145) to accept
and use the requested provider, update the corresponding Redis lookup in
modelexpress_server/src/registry/backend/redis.rs (lines 244-273) to query that
provider’s key rather than the first non-empty candidate, and update the
download-status polling call in modelexpress_server/src/services.rs (lines
828-835) to pass the active provider.
Comment on lines
+176
to
+187
| match self.api().create(&PostParams::default(), &new_cr).await { | ||
| Ok(_) => { | ||
| self.patch_status( | ||
| scoped_name, | ||
| Some(Self::phase_from_status(Self::status_from_phase( | ||
| &legacy_status.phase, | ||
| ))), | ||
| legacy_status.last_used_at.as_deref(), | ||
| legacy_status.created_at.as_deref(), | ||
| Some(legacy_status.message.as_deref()), | ||
| ) | ||
| .await?; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Roll back the scoped CR when legacy status migration fails.
If patch_status fails after creation, the empty scoped CR remains. Future claims hit the fast path and treat it as perpetually DOWNLOADING, bypassing legacy adoption.
Proposed rollback
- self.patch_status(
+ if let Err(patch_err) = self.patch_status(
scoped_name,
Some(Self::phase_from_status(Self::status_from_phase(
&legacy_status.phase,
))),
legacy_status.last_used_at.as_deref(),
legacy_status.created_at.as_deref(),
Some(legacy_status.message.as_deref()),
)
- .await?;
+ .await {
+ if let Err(delete_err) = self.api()
+ .delete(scoped_name, &kube::api::DeleteParams::default())
+ .await
+ {
+ warn!("Failed to roll back {scoped_name}: {delete_err}");
+ }
+ return Err(patch_err);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| match self.api().create(&PostParams::default(), &new_cr).await { | |
| Ok(_) => { | |
| self.patch_status( | |
| scoped_name, | |
| Some(Self::phase_from_status(Self::status_from_phase( | |
| &legacy_status.phase, | |
| ))), | |
| legacy_status.last_used_at.as_deref(), | |
| legacy_status.created_at.as_deref(), | |
| Some(legacy_status.message.as_deref()), | |
| ) | |
| .await?; | |
| match self.api().create(&PostParams::default(), &new_cr).await { | |
| Ok(_) => { | |
| if let Err(patch_err) = self.patch_status( | |
| scoped_name, | |
| Some(Self::phase_from_status(Self::status_from_phase( | |
| &legacy_status.phase, | |
| ))), | |
| legacy_status.last_used_at.as_deref(), | |
| legacy_status.created_at.as_deref(), | |
| Some(legacy_status.message.as_deref()), | |
| ) | |
| .await { | |
| if let Err(delete_err) = self | |
| .api() | |
| .delete(scoped_name, &kube::api::DeleteParams::default()) | |
| .await | |
| { | |
| warn!("Failed to roll back {scoped_name}: {delete_err}"); | |
| } | |
| return Err(patch_err); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelexpress_server/src/registry/backend/kubernetes.rs` around lines 176 -
187, Update the successful create branch in the `self.api().create` flow to roll
back the newly created scoped CR when `patch_status` fails: capture the patch
error, delete the created resource using the existing Kubernetes API and scoped
resource identity, then propagate the original failure. Preserve the current
status migration and success behavior when `patch_status` succeeds.
Comment on lines
+518
to
+540
| async fn delete_model( | ||
| &self, | ||
| request: Request<DeleteModelRequest>, | ||
| ) -> Result<Response<DeleteModelResponse>, Status> { | ||
| let delete_request = request.into_inner(); | ||
|
|
||
| let grpc_provider = GrpcModelProvider::try_from(delete_request.provider).map_err(|_| { | ||
| Status::invalid_argument(format!( | ||
| "Invalid provider value: {}", | ||
| delete_request.provider | ||
| )) | ||
| })?; | ||
| let provider = ModelProvider::from(grpc_provider); | ||
| let model_name = download::canonical_model_name(&delete_request.model_name, provider) | ||
| .map_err(|e| Status::invalid_argument(e.to_string()))?; | ||
|
|
||
| let Some(tracker) = model_tracker() else { | ||
| return Err(Status::unavailable( | ||
| "server startup incomplete: model tracker not initialized", | ||
| )); | ||
| }; | ||
|
|
||
| tracker.delete_status(&model_name).await; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The provider-bearing deletion RPC is implemented as an all-provider deletion. Clearing one provider can invalidate unrelated cached models sharing the same name.
modelexpress_server/src/services.rs#L518-L540: passproviderthroughdelete_statusand the registry manager.modelexpress_server/src/registry/backend/kubernetes.rs#L426-L439: delete only the provider-scoped CR and a matching-provider legacy CR.modelexpress_server/src/registry/backend/redis.rs#L322-L326: delete only the provider-scoped key and a matching-provider legacy key.workspace-tests/tests/registry_backend_redis.rs#L423-L426: assert that the unselected provider record remains.
📍 Affects 4 files
modelexpress_server/src/services.rs#L518-L540(this comment)modelexpress_server/src/registry/backend/kubernetes.rs#L426-L439modelexpress_server/src/registry/backend/redis.rs#L322-L326workspace-tests/tests/registry_backend_redis.rs#L423-L426
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelexpress_server/src/services.rs` around lines 518 - 540, The
provider-bearing delete_model RPC currently deletes all records for a model
name; propagate provider through delete_status and the registry manager so
deletion is provider-scoped. In modelexpress_server/src/services.rs:518-540
update delete_model’s call chain to pass provider; in
modelexpress_server/src/registry/backend/kubernetes.rs:426-439 and
modelexpress_server/src/registry/backend/redis.rs:322-326 delete only the
provider-scoped record plus any matching-provider legacy record; in
workspace-tests/tests/registry_backend_redis.rs:423-426 assert the unselected
provider record remains.
Comment on lines
+540
to
+546
| tracker.delete_status(&model_name).await; | ||
| info!("Deleted registry record for model '{model_name}'"); | ||
|
|
||
| Ok(Response::new(DeleteModelResponse { | ||
| success: true, | ||
| message: Some(format!("Model '{model_name}' removed from registry")), | ||
| })) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Registry deletion failures are suppressed. The RPC reports success after backend failure, while stale recovery retries against the undeleted record and may return the same false DOWNLOADED result.
modelexpress_server/src/services.rs#L540-L546: return a gRPC error when registry deletion fails.modelexpress_server/src/services.rs#L749-L750: retry claiming only after deletion succeeds.
📍 Affects 1 file
modelexpress_server/src/services.rs#L540-L546(this comment)modelexpress_server/src/services.rs#L749-L750
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelexpress_server/src/services.rs` around lines 540 - 546, The model
deletion RPC currently ignores failures from tracker.delete_status, so update
the deletion flow to propagate that failure as an appropriate gRPC error and
only report success after deletion completes. In
modelexpress_server/src/services.rs:540-546, handle the result of
tracker.delete_status; in modelexpress_server/src/services.rs:749-750, ensure
stale recovery retries claiming only after deletion succeeds.
Contributor
|
https://github.com/vllm-project/vllm/pull/48507/changes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
vllm: 0.23.0
humming: 0.1.10(humming_kernels-0.1.10-py3-none-any.whl)
modelexpress: 0.4.1 (modelexpress-0.4.1-cp312-cp312-manylinux_2_24_x86_64.manylinux_2_28_x86_64.whl)
RDMA target rejected or rebuilt the wrong tensor layout.
Some source tensors were published as dense BF16
UnquantizedLinearMethodweights, while the target dummy-load path createdpacked Humming
int32weights for the same module. Before this fix thetarget could not safely decide whether to rebuild, keep, or reject the local
tensor.
Target forward path could still use Humming kernels after receiving dense BF16 weights.
Even after the target tensor shape/dtype was rebuilt to match the source,
the target module could keep its local Humming
quant_method. vLLM theninterpreted a dense BF16 tensor as a packed Humming tensor and failed during
profile/warmup.
RDMA target could publish a polluted manifest after a successful load.
The target creates post-load derived runtime tensors such as MLA attention
tensors (
W_UV,W_UK_T) before RDMA receive. Those tensors are localtarget artifacts and should not become part of the RDMA source manifest.
Before the final cleanup the target manifest grew from the source's
1554tensors to2091tensors.Summary by CodeRabbit
New Features
model clearandmodel clear-allnow remove server-side registry entries when possible.Bug Fixes
Documentation
modelexpress, while retainingmxcompatibility.MX_SERVER_ADDRESSand deprecatedMODEL_EXPRESS_URL.Chores