Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions .github/workflows/modelexpress-ci-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@
# interchangeable copies at worker_rank 0, tp_size=1); EP
# uses expert parallelism (--enable-expert-parallel over a
# TP=2 MoE world, rank-distinct weights, tp_size=2, the
# identity hash carrying expert_parallel_size > 0). See
# identity hash carrying a derived expert-parallel world
# size of tp * dp * pcp rather than a constant). See
# TEST_PLAN rows 5.2 and 5.3.
#
# test-p2p-mla-model — vLLM + SGLang MLA/MoE P2P test on the full-GPU A100b
Expand Down Expand Up @@ -720,9 +721,11 @@ jobs:
#
# EP (--enable-expert-parallel over TP=2, MoE model): experts shard per rank,
# so weights stay rank-distinct and transfers are rank-paired like TP
# (tp_size=2). The new coverage is the identity hash carrying
# expert_parallel_size > 0 (TEST_PLAN row 5.3) — a source/target mismatch there
# forces a disk fallback and fails the RDMA marker assertion.
# (tp_size=2). The new coverage is the identity hash carrying an
# expert-parallel world size derived as tp * dp * pcp rather than read from a
# ParallelConfig attribute that does not exist (TEST_PLAN row 5.3, MX-440) —
# a source/target mismatch there forces a disk fallback and fails the RDMA
# marker assertion.
#
# Both run single-node on the scarce full-GPU non-MIG pool (4 GPUs each), so
# matrix max-parallel: 1 serializes them back-to-back to halve peak demand
Expand Down
63 changes: 51 additions & 12 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,28 @@ Files to bump:
the workspace; chart image tag does not — see below).
- `docs/metadata.md` — `mx_version` example values (search the file for
the old version literal; expect ~2 hits).
- `modelexpress_server/src/p2p/{source_identity,state,service}.rs` and
`modelexpress_server/src/p2p/backend/redis.rs` — `mx_version` strings
in `mod tests` fixtures.
- `modelexpress_client/python/tests/test_source_id.py` and
`test_k8s_service_client.py` — `mx_version` strings in test fixtures.
- `modelexpress_server/src/p2p/{source_identity,state,service}.rs`,
`modelexpress_server/src/p2p/backend/{redis,memory}.rs` and
`workspace-tests/tests/artifact_transfer_contract.rs` — `mx_version`
strings in `mod tests` fixtures. `redis.rs` has two, a fixture and its
`assert_eq!`.
- `modelexpress_client/python/tests/{test_source_id,test_k8s_service_client,
test_vllm_artifacts,test_artifact_transfer}.py` — `mx_version` strings in
test fixtures. `test_vllm_artifacts.py` has three, `test_artifact_transfer.py`
has four.

Do not work from that list alone. It has been incomplete before, and a
missed fixture is silent: `backend/memory.rs` sat at `"0.3.0"` through
several bumps because it was not named here. Bump by search, not by list:

```bash
grep -rIn --exclude-dir=.git '<old_version>' .
```
Comment thread
nicolasnoble marked this conversation as resolved.

Search every file type, not just `*.rs`/`*.py`/`*.toml`: the version also
lives in Markdown, YAML and the Helm chart. Then classify the hits, because
the public-image tag references below are stale on purpose and must not be
swept up by the same pass.

`mx_version` is part of the `SourceIdentity` proto, which is hashed into
the `mx_source_id`. After bumping the version literals, the pinned
Expand All @@ -111,21 +128,43 @@ failing tests:

```bash
cd modelexpress_client/python && .venv/bin/python -m pytest \
tests/test_source_id.py -v -k "pinned_hash or case_colliding"
tests/test_source_id.py -v
```

Update the three Python assertions in `tests/test_source_id.py` and the
three matching Rust assertions in
`modelexpress_server/src/p2p/source_identity.rs` (search for
`test_python_cross_check_*`) so both sides cross-check on the new hashes.

Verify end-to-end before committing:
There are **four** pinned assertions per side, not three, carrying three
distinct hash values. Update all four in `tests/test_source_id.py` and the
four matching ones in `modelexpress_server/src/p2p/source_identity.rs` so
both sides cross-check on the new hashes:

- `test_empty_artifact_fields_preserve_existing_id` (both sides) — shares
the base-identity hash literal. Run the whole file rather than filtering
on `-k "pinned_hash or case_colliding"`, which does not select this one
and leaves it red on both sides.
- `test_pinned_hash_base_identity` / `test_python_cross_check_base_identity`
- `test_pinned_hash_with_revision` / `test_python_cross_check_with_revision`
- `test_case_colliding_extra_parameters_are_deterministic` /
`test_python_cross_check_case_colliding_extra`

If the Rust and Python values ever disagree, stop rather than pinning two
numbers — the canonicalizers in `metadata/source_id.py` and
`p2p/source_identity.rs` have diverged and that is the bug.
Comment thread
nicolasnoble marked this conversation as resolved.

Verify end-to-end before committing. The Python venv needs the `dev` extra
(`uv pip install -e ".[dev]"`); without it `tests/test_tracing.py` aborts
collection on a missing `opentelemetry` and pytest runs zero tests while
reporting no failures.

```bash
cargo check --workspace --tests
cargo test --workspace
cd modelexpress_client/python && .venv/bin/python -m pytest tests/
```

`cargo check --workspace --tests` compiles the test targets, it does not run
their assertions, so on its own it cannot tell you a pinned hash is wrong.
`cargo test --workspace` is the step that actually executes them. Read the
per-test ok lines rather than the exit code alone.

### Public-image tag references — separate cadence

These references point at the public release container on NGC. They lag
Expand Down
2 changes: 1 addition & 1 deletion ci/TEST_PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Coverage matrix for the ModelExpress CI infrastructure. Reflects what's currentl
| 4.3 | Source Bootstrap — GCS | vLLM, TRT-LLM, SGLang | **Not started** | GCS bucket reachable from K8s nodes (workload identity or a service-account key secret) |
| 5.1 | Parallelism > 1 — TP (multi-node preferred) | vLLM, TRT-LLM, SGLang | **In CI (vLLM, single-node + multi-node); TRT-LLM/SGLang follow-ups** — single-node TP=2 coverage is provided by the `test-p2p-mla-model` job (it runs `manifest-azure-tp2.yaml` + `test_p2p_k8s.py` on the non-MIG a100b pool with TP=2, plus VRAM parity); the dedicated Qwen single-node entry was dropped from the `test-tp` matrix to avoid doubling scarce a100b demand (two jobs each needing 4 full GPUs on a 16-GPU pool). The non-MIG pool is required because `nvidia.com/gpu: 2` on a MIG-sliced node returns 2 slices, not 2 GPUs, and NVIDIA hardware blocks NVML topology / NCCL / NVLink across slices. `test-tp` now carries only the multi-node variant (`manifest-azure-multi-node-tp2.yaml`) — StatefulSet + headless Service + Ray, 1 GPU per pod on a100a, 4 pods total (2 source + 2 target), anti-affinity uses `matchLabelKeys` on `apps.kubernetes.io/pod-index` to cross-pair ranks so it fits on a 2-node pool; being 1-GPU-per-pod on a100a it does not contend for a100b. The underlying `device_id` bug (rank-1 NIXL registration failing with `CUDA error: invalid device ordinal` because `adapter.py:_get_vllm_device_id` used global rank instead of local) was fixed upstream in PR #270; the supporting CI plumbing — envsubst allowlist in `run-mx-p2p-test`, ordinal-aware `_pod_name`, multi-pod log scanning in `test_p2p_k8s.py`, container name aligned to `${JOB_NAME}` so `kubectl logs -c mx-target` resolves on StatefulSet pods — landed alongside enabling the matrix entry. TRT-LLM/SGLang TP=2 variants are follow-ups | TRT-LLM/SGLang variants only |
| 5.2 | Parallelism > 1 — DP | vLLM, TRT-LLM, SGLang | **In CI (vLLM single-node DP=2); TRT-LLM/SGLang follow-ups** — `test-dp` matrix job runs vLLM native data parallelism (`--data-parallel-size 2`, `--tensor-parallel-size 1`) via `manifest-azure-dp2.yaml`, pinned to the same non-MIG `a100b` pool as `test-tp` (DP wants one full GPU per core; MIG slices break vLLM's DP coordination). Crucially this runs with `tp_size=1`, NOT 2: DP cores are full, interchangeable model copies and `build_source_identity` carries no DP dimension (tp/pp/ep only), so every DP core resolves to `worker_rank` 0. The source publishes a single rank-0 ModelMetadata CR and the target's DP cores each pull the full rank-0 weight set over RDMA — N concurrent pulls from one source, not rank-paired transfers. The per-rank agent assertion therefore correctly expects exactly one distinct source agent (rank 0). Reuses `run-mx-p2p-test` + `test_p2p_k8s.py` unchanged. TRT-LLM/SGLang DP variants are follow-ups | (a) Per-replica inference assertion (row 5.2 ideal) is not achievable with vLLM native DP: the deployment exposes a single HTTP endpoint that load-balances across DP cores, so individual cores can't be addressed from outside — the existing target inference assertion covers "the DP deployment serves." A true per-replica check would need a router-fronted multi-Deployment topology (or Dynamo DP, where replicas are first-class). (b) TRT-LLM/SGLang DP variants |
| 5.3 | Parallelism > 1 — EP | vLLM, TRT-LLM, SGLang | **In CI (vLLM single-node EP=2); TRT-LLM/SGLang follow-ups** — `test-ep` matrix job runs vLLM expert parallelism (`--tensor-parallel-size 2 --enable-expert-parallel`) on an MoE model (`deepseek-ai/DeepSeek-V2-Lite`, the same model the MLA job uses) via `manifest-azure-ep2.yaml`, pinned to the non-MIG `a100b` pool. In vLLM, EP layers on top of the TP world: attention stays TP-sharded while the MoE experts are distributed expert-parallel across the same ranks, so each rank owns distinct weights, the source publishes one CR per rank, and the target pairs rank-for-rank — hence `tp_size=2` and the per-rank agent assertion expects two distinct source agents (ranks 0,1), identical in shape to the TP test. The coverage beyond `test-tp` is the identity hash: enabling EP sets `parallel_config.expert_parallel_size > 0`, which `build_source_identity` folds into `mx_source_id` — this is the first job to exercise `rdma_strategy` rank pairing with `expert_parallel_size > 0` (a source/target mismatch in that field would force a disk fallback and fail the marker assertion). Reuses `run-mx-p2p-test` + `test_p2p_k8s.py` unchanged | (a) `TP=1 × EP=N` pure-EP shape (vLLM expresses EP over the TP/DP world, so EP>1 needs `TP*DP>1`; the single-pod TP=2+EP shape is the simplest that exercises `expert_parallel_size > 0`). (b) Per-expert-subset transfer assertion — current coverage confirms rank-paired transfer + correct identity hash, not that rank N pulled only expert subset N. (c) TRT-LLM/SGLang EP variants; TP × EP multi-node combo |
| 5.3 | Parallelism > 1 — EP | vLLM, TRT-LLM, SGLang | **In CI (vLLM single-node EP=2); TRT-LLM/SGLang follow-ups** — `test-ep` matrix job runs vLLM expert parallelism (`--tensor-parallel-size 2 --enable-expert-parallel`) on an MoE model (`deepseek-ai/DeepSeek-V2-Lite`, the same model the MLA job uses) via `manifest-azure-ep2.yaml`, pinned to the non-MIG `a100b` pool. In vLLM, EP layers on top of the TP world: attention stays TP-sharded while the MoE experts are distributed expert-parallel across the same ranks, so each rank owns distinct weights, the source publishes one CR per rank, and the target pairs rank-for-rank — hence `tp_size=2` and the per-rank agent assertion expects two distinct source agents (ranks 0,1), identical in shape to the TP test. The coverage beyond `test-tp` is the identity hash: enabling EP sets `parallel_config.enable_expert_parallel`, from which `build_source_identity` derives an expert-parallel world size of `tp * dp * pcp` and folds that into `mx_source_id` — this is the first job to exercise `rdma_strategy` rank pairing with a derived expert-parallel size above 1 (a source/target mismatch in that field would force a disk fallback and fail the marker assertion). vLLM's `ParallelConfig` carries no `expert_parallel_size` attribute; before MX-440 the published value was a constant 0 for every vLLM source, so this job passed with source and target publishing the same wrong number. Reuses `run-mx-p2p-test` + `test_p2p_k8s.py` unchanged | (a) `TP=1 × EP=N` pure-EP shape (vLLM expresses EP over the TP/DP world, so EP>1 needs `TP*DP>1`; the single-pod TP=2+EP shape is the simplest that exercises a derived expert-parallel size above 1). (b) Per-expert-subset transfer assertion — current coverage confirms rank-paired transfer + correct identity hash, not that rank N pulled only expert subset N. (c) TRT-LLM/SGLang EP variants; TP × EP multi-node combo |
| 6 | Model Streamer — S3 *(direct streaming, no server cache)* | vLLM, SGLang | **In CI** — direct-streaming matrix entries run for vLLM and SGLang on `prod-modelexpress-tester-amd-gpu-v1` in a single Docker container with no vCluster or MX server. IRSA provides S3 access; the first run uploads safetensors to `s3://ai-dynamo-modelexpress-ci/models/...`. The shared composite action dispatches vLLM's `modelexpress` load format or SGLang's `remote_instance` + `backend=modelexpress` command, then verifies the MX ModelStreamer completion marker, server health, and inference. TRT-LLM remains gated because it has no `build_model_streamer_weight_iter` adapter ([MX-299](https://linear.app/nvidia/issue/MX-299/trt-llm-adapter)). | — |
| 7 | Dynamo Integration (without Disaggregated Serving) | vLLM, TRT-LLM, SGLang | **In CI (vLLM); TRT-LLM/SGLang follow-ups** — vLLM aggregated serving active as the `aggregated` matrix entry of `test-with-dynamo` in `modelexpress-ci-tests.yml`, with `dynamo-vllm` in the `build-client` matrix. Dynamo operator installed cluster-wide in `dynamo-system` by `setup-mx-vcluster`; per-run action applies a `DynamoGraphDeployment` (ModelExpress + Frontend + VllmWorker, Qwen2.5-0.5B, TP=1, replicas=1 then scaled to 2). DGD name parameterized end-to-end (`mx-dynamo-${{ github.run_id }}`) so concurrent runs don't collide. TRT-LLM/SGLang variants are follow-ups | TRT-LLM/SGLang variants only |
| 8 | Disaggregated Serving | vLLM, TRT-LLM, SGLang | **In CI (vLLM); TRT-LLM/SGLang follow-ups** — vLLM disaggregated serving active as the `disaggregated` matrix entry of `test-with-dynamo`. Manifest at `ci/k8s/client/vllm/dynamo/manifest-azure-disaggregated.yaml` splits the single VllmWorker into VllmPrefillWorker + VllmDecodeWorker; test scales VllmDecodeWorker to 2 replicas, exercising both the prefill↔decode KV-transfer path (unique to disagg) and decode↔decode P2P weight transfer. Shares the `run-mx-dynamo-test` action with row 7 via a `mode` input that drives the wait gates and the scale-patch target. TRT-LLM/SGLang disagg variants are follow-ups; TP × disagg combo is long-term | TRT-LLM/SGLang variants; TP × disagg combo |
Expand Down
17 changes: 11 additions & 6 deletions ci/k8s/client/vllm/manifest-azure-ep2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@
# runs with tp_size=2 and the per-rank agent assertion expects two distinct
# source agents (ranks 0 and 1).
#
# The novel coverage is the identity hash: enabling EP sets
# parallel_config.expert_parallel_size > 0, which build_source_identity
# folds into mx_source_id. This is the first test to exercise rank pairing
# with expert_parallel_size > 0 (TEST_PLAN row 5.3). If that field were
# The novel coverage is the identity hash. vLLM's ParallelConfig has no
# expert_parallel_size attribute, so build_source_identity derives the
# expert-parallel world size as tp * dp * pcp when enable_expert_parallel is
# set and 1 otherwise, then folds that into mx_source_id. This is the first
# test to exercise rank pairing with a derived expert-parallel size above 1
# (TEST_PLAN row 5.3). Before MX-440 the published value was a constant 0 for
# every vLLM source, so this job passed trivially: both sides published the
# same wrong number and the hashes matched. If that field were
# encoded inconsistently between source and target, the source_id would not
# match, the target would fall back to disk, and the RDMA marker assertion
# would fail.
Expand Down Expand Up @@ -104,8 +108,9 @@ spec:
- --tensor-parallel-size
- "2"
# Distribute MoE experts expert-parallel across the TP=2 ranks.
# Sets parallel_config.expert_parallel_size > 0, which flows into
# the ModelExpress source identity (mx_source_id) hash.
# Sets parallel_config.enable_expert_parallel, from which
# build_source_identity derives an expert-parallel world size of
# tp * dp * pcp = 2, folded into the source identity hash.
- --enable-expert-parallel
# Keep KV fixed and small so VRAM parity stays sensitive to
# weight residency instead of vLLM auto-sizing cache around leaks.
Expand Down
2 changes: 1 addition & 1 deletion docs/metadata.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ Every source is identified by a `SourceIdentity` proto containing all fields tha
| `backend_framework` | `VLLM`, `SGLANG`, `TRT_LLM` | Inference framework |
| `tensor_parallel_size` | `8` | TP degree |
| `pipeline_parallel_size` | `2` | PP degree |
| `expert_parallel_size` | `4` | EP degree (MoE models) |
| `expert_parallel_size` | `4` | EP world size, `1` when expert parallelism is off (MoE models) |
| `dtype` | `"bfloat16"` | Weight data type |
| `quantization` | `"fp8"`, `""` | Quantization method |
| `extra_parameters` | `{}` | Framework-specific config |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
from ...accelerators import accelerator_backend_for
from ...load_strategy.context import LoadContext, LoadResult
from ...metadata.client_factory import create_metadata_client
from ...metadata.publish import build_source_identity
from ...rank_utils import get_global_rank
from ...tensor_utils import adopt_hidden_tensors, capture_tensor_attrs, collect_module_tensors
from .source_identity import build_source_identity

logger = logging.getLogger("modelexpress.engines.vllm.adapter")

Expand Down
Loading
Loading