Repository navigation
refactor(e2e): remove parallel testing infrastructure and simplify architecture - #643
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR removes parallel/test orchestration and thread-safety primitives from the e2e test suite, replaces the ModelPool/GPUAllocator with a Worker/start_workers API and process-level port utilities, and simplifies logging, fixture lifecycles, and pytest CI flags to drive sequential test runs and class-scoped backend setup. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant Fixtures as setup_backend / tests
participant WorkerAPI as start_workers / stop_workers
participant Worker as Worker(process)
participant Gateway as Gateway
participant ProcessUtils as get_open_port/release_port
rect rgba(180,160,240,0.5)
TestRunner->>Fixtures: request backend (class-scoped)
Fixtures->>ProcessUtils: get_open_port()
ProcessUtils-->>Fixtures: assigned port
Fixtures->>WorkerAPI: start_workers(model_id, engine, port, count)
WorkerAPI->>Worker: spawn process (cmd, env)
Worker->>WorkerAPI: wait for health → ready
WorkerAPI-->>Fixtures: list[Worker]
Fixtures->>Gateway: Gateway.start(workers=list[Worker])
Gateway-->>TestRunner: gateway endpoint ready
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
dependencies
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the end-to-end (E2E) testing infrastructure by removing all components related to parallel execution. The primary goal is to simplify the codebase, making it easier to understand and maintain, now that E2E tests run sequentially. This change impacts how GPU resources are managed, how model instances are handled, and how logging is configured, resulting in a more streamlined and less complex testing environment. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a substantial and well-executed refactoring to remove the parallel testing infrastructure from the E2E test suite. The changes consistently remove threading, locking, and reference counting mechanisms across multiple files, resulting in a significantly simpler and more maintainable codebase. The architectural simplifications, such as flattening _unlocked method patterns and consolidating logging, are excellent improvements. I've found one area in the refactored ModelPool where robustness could be improved by handling dead workers more gracefully instead of failing the test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/pool.py`:
- Around line 120-125: The ModelPool returned from the no-requirements branch is
created and returned without registering its shutdown/cleanup handler (variable
_model_pool / class ModelPool), and other startup paths (e.g., the block
creating worker processes between lines ~132-149) may fail before cleanup is
registered; fix by always registering the ModelPool shutdown/finalizer (or
GPUAllocator cleanup) immediately after constructing _model_pool and before any
early return, and wrap startup actions that can raise in try/except/finally so
that on failure you call the same cleanup/unregister logic (and then re-raise)
to avoid leaking worker processes/GPU state; apply the same pattern to the other
code path that creates workers/allocators.
In `@e2e_test/fixtures/setup_backend.py`:
- Around line 509-513: The check currently only fails when instances is empty,
allowing partial launches; update the validation to verify that len(instances)
== num_workers (or at least len(instances) >= num_workers if over-provisioning
is allowed) and call pytest.fail with a clear message including model_id,
num_workers and actual count when insufficient workers are returned; adjust the
block that sets worker_urls and model_path (references: instances, num_workers,
model_id, worker_urls, model_path) to only proceed when the worker count meets
this requirement.
- Around line 172-209: The fixture leaves the last cached generator (_cached of
type _CachedBackend) open at session end; fix by adding teardown logic after the
yield to close and clear the cache: after yielding value, in the fixture's
finally/teardown section check if _cached is not None and call
_cached.gen.close() and set _cached = None (and optionally log), ensuring the
generator's finally block runs; locate where _cached is set (the block that
assigns _cached = _CachedBackend(...)) and add this post-yield cleanup there so
the final cached backend is closed at session end.
- Around line 335-339: The validation for PD worker counts currently only fails
when prefills or decodes are empty; update the conditional that checks prefills
and decodes so it compares their lengths against the requested counts
(num_prefill and num_decode) instead of truthiness. In the block that references
prefills, decodes, runtime_label, num_prefill and num_decode, change the
condition to trigger pytest.fail when len(prefills) < num_prefill or
len(decodes) < num_decode and keep the failure message using those variables to
show actual versus expected counts.
In `@e2e_test/infra/model_pool.py`:
- Around line 1415-1425: The current gRPC worker launch can double-release or
leak GPU slots: remove the unconditional allocator.release_slot(gpu_slot) in the
"if instance is None" branch (because _launch_grpc_worker/_evict_instance
already handles cleanup), and instead wrap the call to _launch_grpc_worker in a
try/except that releases the slot only if _launch_grpc_worker raises before it
can do its own cleanup; ensure that when _launch_grpc_worker returns None you
raise the RuntimeError without releasing the slot locally (to avoid
double-release), and use allocator.release_slot(gpu_slot) in the except block to
cover the pre-return exception path; reference symbols: _launch_grpc_worker,
_evict_instance, allocator.release_slot, runtime_label, instance.
- Around line 1541-1564: The code in launch_workers calls allocate_slots(...)
and then proceeds to launch each worker using slot_map.get(w.key), but it
doesn't guard against partial allocations; before looping and calling
self._launch_model, verify that every valid worker has an allocated slot (e.g.,
build missing = [w for w in valid_workers if w.key not in slot_map] or compare
len(slots) vs len(valid_workers)), and if any are missing raise a clear
RuntimeError and do not call self._launch_model (optionally release allocated
slots if your allocator exposes a release/free method); ensure this check is
placed immediately after slot_map is built so slot_map.get(w.key) will never
return None during launches.
- Around line 890-906: The code allocates a GPU slot then calls
_launch_model(model_id, mode, gpu_slot=gpu_slot) but omits the correct worker
variant and instance key, causing _wait_for_instance(key) to block on a
different instance; update the call to _launch_model to pass the computed key
(instance_key=key) and the correct worker_type derived from mode (e.g.,
worker_type=DECODE or PREFILL when mode indicates those variants) so the
launched worker matches the awaited instance; ensure references to
get_model_spec, allocator.allocate_slots, gpu_slot, mode, key, _launch_model and
_wait_for_instance are used to find and patch the call site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ef0065b-ceef-4f0d-868a-25508c13b535
📒 Files selected for processing (14)
.github/workflows/pr-test-rust.ymle2e_test/bindings_go/conftest.pye2e_test/chat_completions/test_validation.pye2e_test/conftest.pye2e_test/embeddings/test_correctness.pye2e_test/fixtures/__init__.pye2e_test/fixtures/hooks.pye2e_test/fixtures/pool.pye2e_test/fixtures/setup_backend.pye2e_test/infra/gpu_allocator.pye2e_test/infra/model_pool.pye2e_test/pyproject.tomle2e_test/responses/test_builtin_tools.pye2e_test/router/test_worker_api.py
💤 Files with no reviewable changes (4)
- e2e_test/responses/test_builtin_tools.py
- e2e_test/pyproject.toml
- e2e_test/fixtures/init.py
- e2e_test/router/test_worker_api.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf50715c9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Address review comments from #643: pool.py: - Register shutdown finalizer before all return paths, not just the main startup path. Early returns (skip_model_pool, no requirements) previously leaked ModelPool without cleanup. - Wrap startup() in try/except to clean up on failure. setup_backend.py: - Add session-scoped autouse fixture to close the last cached backend at session end. Previously the final cached generator's finally block never ran, leaking gateway processes and ports. - Strengthen PD worker count validation: check len(prefills) < num_prefill instead of just truthiness, catching partial launches. - Strengthen local multi-worker validation: check len(instances) < num_workers instead of just emptiness. model_pool.py: - Pass worker_type and instance_key in get()'s _launch_model call so non-REGULAR worker types (prefill/decode) are launched correctly. - Fix gRPC slot double-release: _launch_grpc_worker already calls _evict_instance (which releases the slot) on internal failure. Remove redundant release_slot from the None path, add try/except for uncaught exceptions. - Guard against partial GPU allocation in launch_workers: check len(slots) == len(valid_workers) and release partial allocations on mismatch. scripts/e2e-test.sh: - Remove --workers 0 flag from benchmarks path (pytest-parallel removed). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/fixtures/setup_backend.py (1)
42-57:⚠️ Potential issue | 🟠 MajorCache key is too coarse and can reuse the wrong backend configuration.
Reuse currently keys only on
(class, param). That can return a stale backend when model/workers/gateway markers differ, and it over-reuses for function tests whererequest.clsisNone.🔧 Suggested fix (key by full setup fingerprint, not only class+param)
class _CachedBackend: - __slots__ = ("gen", "value", "cls", "param") + __slots__ = ("gen", "value", "key") - def __init__(self, gen, value, cls, param): + def __init__(self, gen, value, key): self.gen = gen self.value = value - self.cls = cls - self.param = param + self.key = key @@ def setup_backend(request: pytest.FixtureRequest, model_pool: ModelPool): @@ cls = request.cls param = request.param + model_id = get_marker_value(request, "model") or os.environ.get(ENV_MODEL, DEFAULT_MODEL) + workers_cfg = get_marker_kwargs( + request, "workers", defaults={"count": 1, "prefill": None, "decode": None} + ) + gateway_cfg = get_marker_kwargs( + request, + "gateway", + defaults={ + "policy": "round_robin", + "timeout": DEFAULT_ROUTER_TIMEOUT, + "extra_args": None, + "log_level": None, + "log_dir": None, + }, + ) + owner = cls if cls is not None else request.node.nodeid + cache_key = ( + owner, + param, + model_id, + tuple(sorted(workers_cfg.items())), + tuple(sorted((k, repr(v)) for k, v in gateway_cfg.items())), + ) - if _cached is not None and _cached.cls is cls and _cached.param == param: + if _cached is not None and _cached.key == cache_key: yield _cached.value return @@ - _cached = _CachedBackend(gen=gen, value=value, cls=cls, param=param) + _cached = _CachedBackend(gen=gen, value=value, key=cache_key)Also applies to: 184-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/setup_backend.py` around lines 42 - 57, The cache currently keyed only by _CachedBackend.cls and _CachedBackend.param is too coarse and can return stale backends; change the cache lookup and storage logic to compute a full setup fingerprint (include request.node.nodeid or request.function for function tests when request.cls is None, plus marker-derived settings such as model, workers, gateway, and any other setup flags) and use that fingerprint as the cache key instead of (cls, param). Update places that set/_cached and compare cached.cls/_cached.param (including the other occurrence around lines 184-210) to derive and compare the same fingerprint, storing it on _CachedBackend (e.g., add a .fingerprint attribute) so reuse only occurs when the full setup matches exactly. Ensure generators (gen) are closed and backends torn down when the fingerprint differs before recreating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/infra/model_pool.py`:
- Around line 904-927: The get() flow can leave a broken entry in self.instances
when the worker dies or fails deep_health_check; before raising in the failure
paths around calls to self._launch_model / self._wait_for_instance and checks
using instance.is_alive() and instance.deep_health_check(), remove/evict the bad
instance from self.instances and release its gpu_slot (and call any instance
cleanup/terminate method if available) so the slot isn't stranded; update the
failure branches (the raises after is_alive and deep_health_check) to perform
the eviction/release first, then raise the RuntimeError.
- Around line 1583-1584: launch_workers() currently returns the local variable
instances which can be stale because _wait_all_healthy() may evict failed
workers from self.instances; after calling self._wait_all_healthy() rebuild or
compute the return list from the authoritative store (self.instances) rather
than returning the prebuilt instances list. Specifically, in launch_workers()
replace the final return of the local instances with a filtered/rebuilt list
derived from self.instances (or by matching instance IDs from instances against
self.instances) so any workers removed by _wait_all_healthy() are not returned.
- Around line 1382-1385: The get_grpc_worker fast-path returns cached instances
from self.instances without validating liveness; change it to check the worker's
health before returning: in get_grpc_worker, when key in self.instances, fetch
inst, perform the existing liveness/health check (the same validation used on
cold-path) and only then update inst.last_used and return it; if the check
fails, remove the dead inst from self.instances and fall through to the
creation/restart logic so a fresh worker is launched.
---
Outside diff comments:
In `@e2e_test/fixtures/setup_backend.py`:
- Around line 42-57: The cache currently keyed only by _CachedBackend.cls and
_CachedBackend.param is too coarse and can return stale backends; change the
cache lookup and storage logic to compute a full setup fingerprint (include
request.node.nodeid or request.function for function tests when request.cls is
None, plus marker-derived settings such as model, workers, gateway, and any
other setup flags) and use that fingerprint as the cache key instead of (cls,
param). Update places that set/_cached and compare cached.cls/_cached.param
(including the other occurrence around lines 184-210) to derive and compare the
same fingerprint, storing it on _CachedBackend (e.g., add a .fingerprint
attribute) so reuse only occurs when the full setup matches exactly. Ensure
generators (gen) are closed and backends torn down when the fingerprint differs
before recreating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdb19a26-9dba-4c28-898b-6072a85dedd7
📒 Files selected for processing (4)
e2e_test/fixtures/pool.pye2e_test/fixtures/setup_backend.pye2e_test/infra/model_pool.pyscripts/e2e-test.sh
Address review comments from #643: pool.py: - Register shutdown finalizer before all return paths, not just the main startup path. Early returns (skip_model_pool, no requirements) previously leaked ModelPool without cleanup. - Wrap startup() in try/except to clean up on failure. setup_backend.py: - Add session-scoped autouse fixture to close the last cached backend at session end. Previously the final cached generator's finally block never ran, leaking gateway processes and ports. - Strengthen PD worker count validation: check len(prefills) < num_prefill instead of just truthiness, catching partial launches. - Strengthen local multi-worker validation: check len(instances) < num_workers instead of just emptiness. model_pool.py: - Pass worker_type and instance_key in get()'s _launch_model call so non-REGULAR worker types (prefill/decode) are launched correctly. - Fix gRPC slot double-release: _launch_grpc_worker already calls _evict_instance (which releases the slot) on internal failure. Remove redundant release_slot from the None path, add try/except for uncaught exceptions. - Guard against partial GPU allocation in launch_workers: check len(slots) == len(valid_workers) and release partial allocations on mismatch. scripts/e2e-test.sh: - Remove --workers 0 flag from benchmarks path (pytest-parallel removed). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
277dbcc to
3bca008
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bca008b3d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/bindings_go/conftest.py (1)
141-162:⚠️ Potential issue | 🟠 MajorAllocate missing worker indices from the occupied keys, not from the current count.
If the live set becomes sparse after an eviction or failed launch,
len(existing_for_mode) + ican reuse an already-running key (for example, only:1survives). That collides withWorkerIdentity.keyand can overwrite an existing worker instead of launching a new one.🔧 Proposed fix
if len(existing_for_mode) >= num_workers: instances = existing_for_mode[:num_workers] else: # Need to launch more workers + existing_indices = { + 0 + if inst.key == f"{model_id}:{ConnectionMode.GRPC.value}" + else int(inst.key.rsplit(":", 1)[1]) + for inst in existing_for_mode + } missing = num_workers - len(existing_for_mode) + new_indices: list[int] = [] + candidate = 0 + while len(new_indices) < missing: + if candidate not in existing_indices: + new_indices.append(candidate) + candidate += 1 workers_to_launch = [ WorkerIdentity( model_id, ConnectionMode.GRPC, WorkerType.REGULAR, - len(existing_for_mode) + i, + index, ) - for i in range(missing) + for index in new_indices ] new_instances = model_pool.launch_workers(workers_to_launch, startup_timeout=300) instances = existing_for_mode + new_instances🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bindings_go/conftest.py` around lines 141 - 162, The launch logic currently uses len(existing_for_mode) to generate new WorkerIdentity indices which can reuse keys when the live set is sparse; instead compute the next free indices from the occupied keys and use those to construct WorkerIdentity instances. Specifically, inspect existing_for_mode to extract each worker's numeric index (from WorkerIdentity.key or the attribute that encodes the instance index), build a set of used indices, then pick the missing indices (e.g., the smallest non-negative integers not in that set) to create WorkerIdentity(...) for model_pool.launch_workers; keep using model_pool.get_workers_by_type, WorkerIdentity, and model_pool.launch_workers but replace len(existing_for_mode) + i with indices derived from the used-index set so you never collide with an existing key.
♻️ Duplicate comments (3)
e2e_test/infra/model_pool.py (3)
1382-1385:⚠️ Potential issue | 🟠 MajorRevalidate cached gRPC workers before returning them.
This fast path can still hand out a dead cached worker. Evict stale instances and fall through to the cold path instead of updating
last_usedunconditionally.🔧 Proposed fix
- if key in self.instances: - inst = self.instances[key] - inst.last_used = time.time() - return inst + if key in self.instances: + inst = self.instances[key] + if inst.is_alive() and inst.health_check(): + inst.last_used = time.time() + return inst + self._evict_instance(key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/infra/model_pool.py` around lines 1382 - 1385, The cached-instance fast path unconditionally returns and updates inst.last_used, which can hand out dead gRPC workers; change this to validate the cached instance before returning: when key is in self.instances, fetch inst, check its liveness/health (e.g., call inst.is_alive() or send a lightweight health/ping), and if the check fails remove/evict the entry from self.instances and let execution fall through to the cold path; only if the instance is healthy update inst.last_used = time.time() and return it. Ensure you reference and update the same self.instances[key] entry and use the existing eviction logic/path for dead workers.
904-923:⚠️ Potential issue | 🟠 Major
get()still leaks broken workers, and the PD cold path is incomplete.This path raises without evicting the failed instance on launch/wait/deep-health failures, so the bad entry and its GPU slot remain in
self.instances. It also omits the prefill/decode bootstrap/IB arguments that the other launch paths pass for PD workers.🔧 Proposed fix
- self._launch_model( - model_id, - mode, - gpu_slot=gpu_slot, - worker_type=worker_type, - instance_key=key, - ) - self._wait_for_instance(key) + bootstrap_port = get_open_port() if worker_type == WorkerType.PREFILL else None + ib_device = ( + detect_ib_device() + if worker_type in {WorkerType.PREFILL, WorkerType.DECODE} + else None + ) + try: + self._launch_model( + model_id, + mode, + gpu_slot=gpu_slot, + worker_type=worker_type, + bootstrap_port=bootstrap_port, + ib_device=ib_device, + instance_key=key, + ) + self._wait_for_instance(key) + except Exception: + self._evict_instance(key) + raise @@ - if not instance.is_alive(): - raise RuntimeError(f"Worker {key} process died (was healthy at startup)") + if not instance.is_alive(): + self._evict_instance(key) + raise RuntimeError(f"Worker {key} process died (was healthy at startup)") @@ - if not instance.deep_health_check(timeout=30.0): - raise RuntimeError( + if not instance.deep_health_check(timeout=30.0): + self._evict_instance(key) + raise RuntimeError( f"Worker {key} failed deep health check (health_generate) - " "model may be stuck or crashed" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/infra/model_pool.py` around lines 904 - 923, In get()’s PD cold-launch path, ensure failed launches are cleaned up and PD workers receive the same bootstrap args as other paths: call self._launch_model(...) with the prefill/decode/bootstrap/ib arguments used elsewhere when launching PD workers (match the parameter names passed in other branches), then after calling self._wait_for_instance(key) wrap the subsequent checks (instance.is_alive() and instance.deep_health_check(...)) in a try/except/finally or check block that on any failure removes the bad entry and frees the GPU slot from self.instances (e.g., pop(self.instances, key) and mark gpu_slot available) before re-raising the RuntimeError so broken workers do not leak. Ensure you reference the same instance key variable and methods _launch_model, _wait_for_instance, is_alive, and deep_health_check when implementing this cleanup.
1567-1584:⚠️ Potential issue | 🟠 Major
launch_workers()needs rollback and an authoritative return set.If
_launch_model()fails partway through the loop, already-launched workers and unused allocated slots are left behind. Even on the non-exception path,_wait_all_healthy()can evict failed workers before you return the localinstanceslist, so callers can receive objects that are no longer present inself.instances.🔧 Proposed fix
instances: list[ModelInstance] = [] - for w in valid_workers: - # Each prefill worker needs its own bootstrap port for PD communication - bootstrap_port = get_open_port() if w.is_prefill else None - - instance = self._launch_model( - model_id=w.model_id, - mode=w.mode, - gpu_slot=slot_map.get(w.key), - worker_type=w.worker_type, - bootstrap_port=bootstrap_port, - ib_device=ib_device if (w.is_prefill or w.is_decode) else None, - instance_key=w.key, - ) - instances.append(instance) - - self._wait_all_healthy() - return instances + launched_keys: set[str] = set() + try: + for w in valid_workers: + # Each prefill worker needs its own bootstrap port for PD communication + bootstrap_port = get_open_port() if w.is_prefill else None + + instance = self._launch_model( + model_id=w.model_id, + mode=w.mode, + gpu_slot=slot_map.get(w.key), + worker_type=w.worker_type, + bootstrap_port=bootstrap_port, + ib_device=ib_device if (w.is_prefill or w.is_decode) else None, + instance_key=w.key, + ) + instances.append(instance) + launched_keys.add(w.key) + + self._wait_all_healthy() + healthy_instances = [self.instances[w.key] for w in valid_workers if w.key in self.instances] + if len(healthy_instances) != len(valid_workers): + missing = [w.key for w in valid_workers if w.key not in self.instances] + raise RuntimeError(f"Some workers failed to become healthy: {missing}") + return healthy_instances + except Exception: + for key in list(launched_keys): + self._evict_instance(key) + for slot in slots: + if slot.assigned_model not in launched_keys: + self.allocator.release_slot(slot) + raise
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/setup_backend.py`:
- Around line 187-210: The current cache reuse logic only checks _cached.cls and
_cached.param so different tests that vary marks like `@pytest.mark.model`,
.workers, .gateway, or .storage (or function tests where cls is None) will
incorrectly reuse the wrong backend; change the cache key to include the fully
resolved backend configuration derived from request (e.g., resolved
model/workers/gateway/storage and request.param) instead of just cls and param:
when creating the backend via _create_backend(request, model_pool) capture the
resolved config used to instantiate the generator and store it on _CachedBackend
(alongside gen and value), then update the reuse check to compare that stored
config to the newly resolved config before reusing or tearing down the cached
backend. Ensure _CachedBackend and the places referencing _cached (the reuse
block and teardown block) use the new config field.
In `@e2e_test/infra/model_pool.py`:
- Around line 1076-1099: The get_workers_by_type function currently returns
entries from self.instances including dead/crashed processes; update its filter
to exclude non-running ModelInstance objects by checking the instance liveness
(e.g., use getattr(inst, "is_alive", None) and if callable call it, or check a
boolean attribute like inst.alive/inst.is_running) so only instances that report
alive/running are included; keep the existing filters on inst.model_id,
inst.worker_type and inst.mode and reference the get_workers_by_type function,
self.instances, and ModelInstance when making this change.
---
Outside diff comments:
In `@e2e_test/bindings_go/conftest.py`:
- Around line 141-162: The launch logic currently uses len(existing_for_mode) to
generate new WorkerIdentity indices which can reuse keys when the live set is
sparse; instead compute the next free indices from the occupied keys and use
those to construct WorkerIdentity instances. Specifically, inspect
existing_for_mode to extract each worker's numeric index (from
WorkerIdentity.key or the attribute that encodes the instance index), build a
set of used indices, then pick the missing indices (e.g., the smallest
non-negative integers not in that set) to create WorkerIdentity(...) for
model_pool.launch_workers; keep using model_pool.get_workers_by_type,
WorkerIdentity, and model_pool.launch_workers but replace len(existing_for_mode)
+ i with indices derived from the used-index set so you never collide with an
existing key.
---
Duplicate comments:
In `@e2e_test/infra/model_pool.py`:
- Around line 1382-1385: The cached-instance fast path unconditionally returns
and updates inst.last_used, which can hand out dead gRPC workers; change this to
validate the cached instance before returning: when key is in self.instances,
fetch inst, check its liveness/health (e.g., call inst.is_alive() or send a
lightweight health/ping), and if the check fails remove/evict the entry from
self.instances and let execution fall through to the cold path; only if the
instance is healthy update inst.last_used = time.time() and return it. Ensure
you reference and update the same self.instances[key] entry and use the existing
eviction logic/path for dead workers.
- Around line 904-923: In get()’s PD cold-launch path, ensure failed launches
are cleaned up and PD workers receive the same bootstrap args as other paths:
call self._launch_model(...) with the prefill/decode/bootstrap/ib arguments used
elsewhere when launching PD workers (match the parameter names passed in other
branches), then after calling self._wait_for_instance(key) wrap the subsequent
checks (instance.is_alive() and instance.deep_health_check(...)) in a
try/except/finally or check block that on any failure removes the bad entry and
frees the GPU slot from self.instances (e.g., pop(self.instances, key) and mark
gpu_slot available) before re-raising the RuntimeError so broken workers do not
leak. Ensure you reference the same instance key variable and methods
_launch_model, _wait_for_instance, is_alive, and deep_health_check when
implementing this cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8265b0b-d382-47ae-977b-91da82dade04
📒 Files selected for processing (15)
.github/workflows/pr-test-rust.ymle2e_test/bindings_go/conftest.pye2e_test/chat_completions/test_validation.pye2e_test/conftest.pye2e_test/embeddings/test_correctness.pye2e_test/fixtures/__init__.pye2e_test/fixtures/hooks.pye2e_test/fixtures/pool.pye2e_test/fixtures/setup_backend.pye2e_test/infra/gpu_allocator.pye2e_test/infra/model_pool.pye2e_test/pyproject.tomle2e_test/responses/test_builtin_tools.pye2e_test/router/test_worker_api.pyscripts/e2e-test.sh
💤 Files with no reviewable changes (4)
- e2e_test/pyproject.toml
- e2e_test/responses/test_builtin_tools.py
- e2e_test/router/test_worker_api.py
- e2e_test/fixtures/init.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/hooks.py`:
- Around line 98-100: Normalize empty routing env vars by coercing any
empty-string values to None before applying selection logic: when reading
_engine, _vendor, and especially _gpu_tier, replace "" with None (or use
truthiness checks) so the GPU branch in the filter does not treat an exported
empty E2E_GPU_TIER as an active selector; update the code around
_engine/_vendor/_gpu_tier initialization and the gpu_count filtering logic
(referencing _gpu_tier and the selection block that checks gpu_count) to use
None-or-truthiness semantics instead of comparing against "".
- Around line 97-123: The selection logic filters only by engine, vendor, and
gpu, so add an environment-driven storage filter: read _storage =
os.environ.get("E2E_STORAGE") alongside _engine/_vendor/_gpu_tier and, if
_storage is set, retrieve storage_marker = item.get_closest_marker("storage")
and require _storage to be present in storage_marker.args (or compare the single
value) before selecting the item; update the selection loop (the same block
using engine_marker/vendor_marker/gpu_marker and appending to items[:]) to
include this storage check so storage-specialized suites like
storage("oracle-custom") are distinguishable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 285c4baa-3489-4f2b-aade-42ea19aaf06d
📒 Files selected for processing (23)
e2e_test/bindings_go/test_go_oai_server.pye2e_test/chat_completions/test_enable_thinking.pye2e_test/chat_completions/test_function_calling.pye2e_test/chat_completions/test_openai_server.pye2e_test/chat_completions/test_reasoning_content.pye2e_test/chat_completions/test_validation.pye2e_test/embeddings/test_basic.pye2e_test/embeddings/test_correctness.pye2e_test/fixtures/hooks.pye2e_test/messages/test_basic.pye2e_test/messages/test_mcp_tool.pye2e_test/messages/test_tool_search.pye2e_test/messages/test_tool_use.pye2e_test/responses/test_basic_crud.pye2e_test/responses/test_builtin_tools.pye2e_test/responses/test_state_management.pye2e_test/responses/test_storage_hooks.pye2e_test/responses/test_streaming_events.pye2e_test/responses/test_structured_output.pye2e_test/responses/test_tools_call.pye2e_test/router/test_mmlu.pye2e_test/router/test_pd_mmlu.pye2e_test/router/test_worker_api.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 761fb087d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/responses/test_builtin_tools.py (1)
133-163:⚠️ Potential issue | 🔴 CriticalDon’t skip or leak resources when the gRPC fixture startup breaks.
Once this class is selected, a worker startup failure is a regression, not a skip. Also, anything that throws after
start_workers()and beforeyieldleaves the worker running because teardown only begins after theyield.🔧 Proposed fix
# Start a gRPC worker try: workers = start_workers("openai/gpt-oss-20b", engine, mode=ConnectionMode.GRPC, count=1) except Exception as e: - pytest.skip(f"gRPC worker not available: {e}") + pytest.fail(f"Failed to start gRPC worker: {e}") @@ - gateway = Gateway() - gateway.start( - worker_urls=[worker.base_url], - model_path=model_path, - extra_args=[ - "--mcp-config-path", - mcp_config_file, - "--reasoning-parser=gpt-oss", - "--history-backend", - "memory", - ], - ) - - client = openai.OpenAI( - base_url=f"{gateway.base_url}/v1", - api_key="not-used", - ) - - yield gateway, client, model_path - - gateway.shutdown() - stop_workers(workers) + gateway = Gateway() + try: + gateway.start( + worker_urls=[worker.base_url], + model_path=model_path, + extra_args=[ + "--mcp-config-path", + mcp_config_file, + "--reasoning-parser=gpt-oss", + "--history-backend", + "memory", + ], + ) + client = openai.OpenAI( + base_url=f"{gateway.base_url}/v1", + api_key="not-used", + ) + yield gateway, client, model_path + finally: + try: + gateway.shutdown() + finally: + stop_workers(workers)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/responses/test_builtin_tools.py` around lines 133 - 163, Remove the pytest.skip on start_workers failure so startup errors surface as test failures; after calling start_workers (function start_workers) ensure any exception thrown later (before yield) does not leak the worker by wrapping the post-start setup (calls to gateway.start and client creation) in a try/except/finally that calls stop_workers(workers) on error, or use a try/finally around the entire pre-yield sequence to guarantee stop_workers(workers) is invoked if gateway.start or other setup raises; keep the normal yield path so existing teardown code (gateway.shutdown and stop_workers) still runs after the yield.
♻️ Duplicate comments (1)
e2e_test/fixtures/hooks.py (1)
61-65:⚠️ Potential issue | 🟠 MajorNormalize blank routing env vars before filtering.
os.environ.get()preserves exported empty strings. If a job setsE2E_ENGINE=sglangandE2E_GPU_TIER="", Line 81 still treats GPU filtering as active and deselects every item because nogpu_countmatches"".🔧 Proposed fix
- engine = os.environ.get("E2E_ENGINE") - vendor = os.environ.get("E2E_VENDOR") - gpu_tier = os.environ.get("E2E_GPU_TIER") + engine = os.environ.get("E2E_ENGINE") or None + vendor = os.environ.get("E2E_VENDOR") or None + gpu_tier = os.environ.get("E2E_GPU_TIER") or None - if not any([engine, vendor, gpu_tier]): + if not any((engine, vendor, gpu_tier)): return @@ - if gpu_tier is not None: + if gpu_tier: gpu_marker = item.get_closest_marker("gpu") gpu_count = gpu_marker.args[0] if gpu_marker else 1 if str(gpu_count) != gpu_tier:Also applies to: 81-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/hooks.py` around lines 61 - 65, Environment variables engine, vendor, and gpu_tier can be exported as empty strings and must be normalized before the any([...]) check and subsequent filtering; update the assignments for engine, vendor, and gpu_tier to strip whitespace and convert empty results to None (e.g., read the var, do value = value.strip() if value and value.strip() else None) so the existing condition if not any([engine, vendor, gpu_tier]) and the filtering logic around gpu_count (lines ~81-85) treat blank exports as unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/conftest.py`:
- Around line 20-23: The docstring incorrectly states that setup_backend is
function-scoped; update the documentation in conftest.py to reflect that the
fixture setup_backend is declared with scope="class" (class-scoped) so the
comment matches the actual fixture behavior and clarifies backend reuse across
tests.
In `@e2e_test/fixtures/setup_backend.py`:
- Around line 161-181: The gateway startup (_start_gateway / gateway.start) is
executed before the try/finally that ensures workers are stopped, so if
gateway.start fails the workers started by start_workers remain running; modify
both _setup_local() and backend_router() to start the gateway inside the try
block (i.e., after start_workers and before yielding) or move the call to
_start_gateway into the try so that the finally always calls gateway.shutdown()
and stop_workers(workers), ensuring stop_workers(workers) runs even if
gateway.start/_start_gateway raises; update references in those functions
(start_workers, _start_gateway, gateway.shutdown, stop_workers) accordingly.
- Around line 213-244: Wrap the gateway start/yield in a try/finally that always
stops any started workers so prefill_workers/decode_workers aren’t leaked if
_start_gateway or subsequent steps raise; specifically ensure all_workers is
defined before attempting gateway startup, call stop_workers(all_workers) in the
outer finally, and only call gateway.shutdown() if gateway was successfully
created/started (guarded by a flag or checking gateway existence). Update the
block around start_workers, _start_gateway, gateway.shutdown, and stop_workers
to guarantee cleanup even on failures.
In `@e2e_test/infra/process_utils.py`:
- Around line 20-23: get_open_port() currently adds allocated ports to the
module-level _reserved_ports set and start_workers() also reserves ports, but
Worker.stop() never releases them so ports remain blocked; update the worker
teardown to remove released ports from _reserved_ports (or change get_open_port
to only temporarily reserve until bind succeeds) by adding logic in
Worker.stop() (or Worker.shutdown/teardown) to call into the same module that
holds _reserved_ports to discard its assigned port(s) (referencing
_reserved_ports, get_open_port, start_workers, and Worker.stop/Worker class) so
stopped workers return their ports back to the allocator.
In `@e2e_test/infra/worker.py`:
- Around line 269-272: The log file is opened without an explicit encoding in
the worker start-up (see log_path and self._log_file assignment and the
open(...) call); update the open(...) call that assigns self._log_file to
include encoding="utf-8" (and optionally errors="replace" if desired) so
stdout_target/stderr_target write a consistent UTF-8 log regardless of platform.
- Around line 396-404: The loop calling worker.start(timeout=timeout) can leave
previously started workers in workers orphaned if an exception occurs; wrap the
worker.start call in a try/except so on any exception you iterate over the
already-started workers list and perform a best-effort cleanup (call the worker
cleanup API such as stop(), shutdown(), or terminate() — use hasattr to pick the
available method and handle/ignore cleanup exceptions), log the cleanup outcome,
and then re-raise the original exception so the failure is propagated; update
the code around worker.start and the workers list to ensure deterministic
cleanup.
In `@e2e_test/router/test_worker_api.py`:
- Around line 216-220: The TestDisableHealthCheck test class (e.g.,
test_disable_health_check_workers_immediately_healthy and the other methods in
that class) lacks the pytest marks used by other E2E classes, so add the same
decorators used elsewhere: annotate the TestDisableHealthCheck class with
`@pytest.mark.engine`(...) and `@pytest.mark.gpu`(...) (matching the pattern used in
the other test classes so it is not deselected when E2E_ENGINE is set); apply
the same change to the other methods in this class (the block around the second
occurrence at lines ~262-268) so the entire class is collected under the new
hook filter.
- Around line 176-209: If start_workers for the gRPC batch can raise and leave
http_workers running, wrap the second start in its own try/except (or use a
nested try/finally) so that on exception you call stop_workers(http_workers)
before re-raising; update construction of all_workers only after both starts
succeed (or ensure stop_workers is idempotent) and keep gateway/start/shutdown
logic unchanged. Specifically, handle exceptions around the call to
start_workers that produces grpc_workers and call stop_workers(http_workers) on
failure to ensure http_workers are cleaned up.
---
Outside diff comments:
In `@e2e_test/responses/test_builtin_tools.py`:
- Around line 133-163: Remove the pytest.skip on start_workers failure so
startup errors surface as test failures; after calling start_workers (function
start_workers) ensure any exception thrown later (before yield) does not leak
the worker by wrapping the post-start setup (calls to gateway.start and client
creation) in a try/except/finally that calls stop_workers(workers) on error, or
use a try/finally around the entire pre-yield sequence to guarantee
stop_workers(workers) is invoked if gateway.start or other setup raises; keep
the normal yield path so existing teardown code (gateway.shutdown and
stop_workers) still runs after the yield.
---
Duplicate comments:
In `@e2e_test/fixtures/hooks.py`:
- Around line 61-65: Environment variables engine, vendor, and gpu_tier can be
exported as empty strings and must be normalized before the any([...]) check and
subsequent filtering; update the assignments for engine, vendor, and gpu_tier to
strip whitespace and convert empty results to None (e.g., read the var, do value
= value.strip() if value and value.strip() else None) so the existing condition
if not any([engine, vendor, gpu_tier]) and the filtering logic around gpu_count
(lines ~81-85) treat blank exports as unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 039553c8-76d3-4472-bf10-b970901fa787
📒 Files selected for processing (15)
e2e_test/bindings_go/conftest.pye2e_test/conftest.pye2e_test/fixtures/__init__.pye2e_test/fixtures/hooks.pye2e_test/fixtures/pool.pye2e_test/fixtures/setup_backend.pye2e_test/infra/__init__.pye2e_test/infra/gateway.pye2e_test/infra/gpu_allocator.pye2e_test/infra/model_pool.pye2e_test/infra/model_specs.pye2e_test/infra/process_utils.pye2e_test/infra/worker.pye2e_test/responses/test_builtin_tools.pye2e_test/router/test_worker_api.py
💤 Files with no reviewable changes (5)
- e2e_test/infra/model_specs.py
- e2e_test/infra/model_pool.py
- e2e_test/fixtures/pool.py
- e2e_test/fixtures/init.py
- e2e_test/infra/gpu_allocator.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f16a1be6e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…obs, mypy errors What changed: - .github/workflows/pr-test-rust.yml: added Set up Python step to e2e-vendor job (pip was not found on k8s-runner-cpu), removed 3 invalid vendor matrix entries (openai-chat-completions, xai-chat-completions, gemini-chat-completions) that had no corresponding tests - e2e_test/fixtures/hooks.py: restored skip_for_runtime marker handling via pytest_runtest_setup hook, registered missing markers (gateway, workers, storage, external, slowtest), normalized empty env vars to None to prevent E2E_GPU_TIER="" from filtering out all tests - e2e_test/fixtures/__init__.py: export pytest_runtest_setup hook - e2e_test/conftest.py: import and re-export pytest_runtest_setup hook - e2e_test/infra/worker.py: fix mypy union-attr error on self.process.pid by guarding against None - e2e_test/infra/gateway.py: fix mypy arg-type error by adding assert to narrow cloud_backend from str | None to str Why: PR #643 had 7 failing CI jobs — all 6 vendor jobs failed with "pip: command not found" (missing setup-python action), and python-lint failed with 2 mypy type errors. Additionally, 3 vendor jobs were testing combinations that don't exist (no cloud-backend chat_completions tests, no gemini tests at all). The skip_for_runtime marker was lost during the hooks.py rewrite and needed to be restored for engine-specific test skips. Refs: #643 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…obs, mypy errors What changed: - .github/workflows/pr-test-rust.yml: added Set up Python step to e2e-vendor job (pip was not found on k8s-runner-cpu), removed 3 invalid vendor matrix entries (openai-chat-completions, xai-chat-completions, gemini-chat-completions) that had no corresponding tests - e2e_test/fixtures/hooks.py: restored skip_for_runtime marker handling via pytest_runtest_setup hook, registered missing markers (gateway, workers, storage, external, slowtest), normalized empty env vars to None to prevent E2E_GPU_TIER="" from filtering out all tests - e2e_test/fixtures/__init__.py: export pytest_runtest_setup hook - e2e_test/conftest.py: import and re-export pytest_runtest_setup hook - e2e_test/infra/worker.py: fix mypy union-attr error on self.process.pid by guarding against None - e2e_test/infra/gateway.py: fix mypy arg-type error by adding assert to narrow cloud_backend from str | None to str Why: PR #643 had 7 failing CI jobs — all 6 vendor jobs failed with "pip: command not found" (missing setup-python action), and python-lint failed with 2 mypy type errors. Additionally, 3 vendor jobs were testing combinations that don't exist (no cloud-backend chat_completions tests, no gemini tests at all). The skip_for_runtime marker was lost during the hooks.py rewrite and needed to be restored for engine-specific test skips. Refs: #643 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
fe62e82 to
5efe73e
Compare
…ailure What changed: - scripts/ci_install_e2e_deps.sh: add `requests` to pip install list — it is imported at module level by infra/process_utils.py and was missing on k8s-runner-cpu (vendor jobs failed with ModuleNotFoundError) - e2e_test/fixtures/setup_backend.py: move gateway startup inside try/finally in _setup_local, _setup_pd, and backend_router so workers are always cleaned up even if gateway.start() raises; guard partial PD launches by accumulating workers into all_workers list before each start_workers call - e2e_test/conftest.py: fix stale docstring (function-scoped → class-scoped) Why: All 3 remaining vendor CI jobs (anthropic-messages, openai-responses, xai-responses) failed with "No module named 'requests'" because the e2e deps script didn't install it. The worker cleanup issue was flagged by PR reviewers — if gateway startup failed, worker processes would leak. Refs: #643 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2142199778
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| uses: ./.github/workflows/e2e-gpu-job.yml | ||
| with: | ||
| engine: trtllm | ||
| gpu_tier: "4" |
There was a problem hiding this comment.
This lane sets E2E_ENGINE=trtllm and E2E_GPU_TIER=4, but the new marker-based filtering in fixtures/hooks.py only keeps tests whose markers match both values; in this commit, trtllm tests are marked gpu(1) while the only gpu(4) test is sglang-only. That leaves this job with zero collected tests, and pytest exits non-zero (NO_TESTS_COLLECTED), turning the lane into a consistent CI failure instead of validation.
Useful? React with 👍 / 👎.
infra/__init__.py unconditionally imports run_eval which imports simple_eval_common, pulling in jinja2 and tqdm at module level. These were available on GPU runners (installed by engine setup) but missing on k8s-runner-cpu used by vendor jobs. Refs: #643 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 679e2cee1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The anthropic-messages vendor job runs MCP tool tests (test_mcp_tool.py, test_tool_search.py) that need Brave MCP server. The old CI had setup_agentic_deps: true for the combined agentic-apis job that ran both responses and messages together. Also bumped timeout from 15 to 20 minutes to match the heavier workload. Refs: #643 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
b0a7eeb to
72dbf42
Compare
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72dbf42384
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ganize CI by GPU+API Remove parallel testing infrastructure and simplify the E2E test architecture. Reorganize CI jobs from engine-first (sglang-1gpu, vllm-1gpu, etc.) to GPU tier + API directory (1gpu-chat, 1gpu-embeddings, 1gpu-router, etc.) with engine as a matrix axis within each job. Key changes: - Remove model pool, parallel worker management, and complex eviction logic - Replace with direct single-worker-per-test-class fixture management - Add pytest markers (@engine, @gpu, @model, @Vendor) to all test classes - Add marker-based collection hooks for filtering by engine/gpu/vendor - Reorganize CI into 7 GPU+API jobs (was 9 engine-first jobs) - Split PD tests into dedicated e2e-2gpu-pd jobs - Drop trtllm-4gpu job (0 tests exist for that combination) - Add test_dirs input to e2e-gpu-job.yml reusable workflow - Fix embedding model spec key to use full HuggingFace model ID CI job structure: 1-GPU: chat (sglang,vllm,trtllm), embeddings (sglang), router (sglang,vllm) 2-GPU: chat (sglang), responses (sglang), pd (sglang,vllm) 4-GPU: chat (sglang) Vendor: anthropic-messages, openai-responses, xai-responses Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
72dbf42 to
8512aab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8512aabef6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| worker.start(timeout=timeout) | ||
| workers.append(worker) | ||
| except Exception: | ||
| stop_workers(workers) |
There was a problem hiding this comment.
start_workers only appends a worker to workers after worker.start() succeeds, so if startup fails (for example health-check timeout, bad launch args, or missing runtime deps) the failing Worker instance is never passed to stop_workers. In that path, any spawned process and reserved ports from get_open_port() are left behind, which can poison later tests with leaked GPU processes and port exhaustion.
Useful? React with 👍 / 👎.
| if self.process is None or self.process.poll() is not None: | ||
| return |
There was a problem hiding this comment.
Worker.stop() returns immediately when self.process.poll() is not None, but port/log cleanup happens later in the method. When a worker crashes before teardown, this early return skips release_port(...), leaving _reserved_ports entries behind and making subsequent get_open_port() calls increasingly likely to fail after repeated crashes.
Useful? React with 👍 / 👎.
…nds, scale down k8s-runner-gpu - Add runner-values-1-gpu.yaml: new runner scale set accepting both H100 and A10 nodes with 1 GPU, used by sglang/vllm 1-GPU E2E jobs - Update pr-test-rust.yml: sglang/vllm use 1-gpu runner, trtllm stays on 1-gpu-h100 (requires sm90/H100), embeddings and gateway use 1-gpu - Add terminationGracePeriodSeconds: 600 to all GPU runner values files to prevent premature SIGKILL during job execution - Scale down k8s-runner-gpu minRunners from 10 to 2 (was consuming 40 idle GPUs) - Update README with new 1-gpu install command Signed-off-by: Simon Lin <simon@lightseek.ai> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
the
documentation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2049f50819
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| worker.start(timeout=timeout) | ||
| workers.append(worker) | ||
| except Exception: | ||
| stop_workers(workers) |
There was a problem hiding this comment.
start_workers only appends a worker after worker.start(...) succeeds, so if startup times out/crashes after spawning the subprocess, the except block only calls stop_workers(workers) on previously appended entries and the failing worker is never terminated or its reserved ports released. In flaky startup scenarios this leaks GPU processes and poisoned port reservations into later tests, causing cascading failures.
Useful? React with 👍 / 👎.
| if self.process is None or self.process.poll() is not None: | ||
| return |
There was a problem hiding this comment.
Worker.stop returns immediately when self.process.poll() is not None, which skips both log-file cleanup and release_port(...). If a worker crashes during a test, its reserved port(s) remain in _reserved_ports, and after enough such crashes get_open_port can fail despite free OS ports, creating avoidable follow-on test failures.
Useful? React with 👍 / 👎.
| }, | ||
| # Embedding model | ||
| "embedding": { | ||
| "intfloat/e5-mistral-7b-instruct": { |
There was a problem hiding this comment.
Changing the embedding spec key to "intfloat/e5-mistral-7b-instruct" removes the "embedding" lookup that hf_reference_embeddings still uses (MODEL_SPECS.get("embedding", {}) in e2e_test/embeddings/test_correctness.py), so that fixture now always calls pytest.skip(...) and the embedding correctness suite is silently skipped instead of validating embedding accuracy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d40870d6f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if model_path is None: | ||
| pytest.skip("Embedding model not found in MODEL_SPECS") | ||
| # Get model path from MODEL_SPECS for the embedding model | ||
| model_path = MODEL_SPECS.get("embedding", {}).get("model") |
There was a problem hiding this comment.
The embedding spec key was renamed to "intfloat/e5-mistral-7b-instruct" in this commit, but this fixture still reads MODEL_SPECS.get("embedding", {}). That makes model_path None, so the fixture calls pytest.skip(...) and the embedding correctness suite is skipped entirely instead of validating output quality, silently removing this CI signal.
Useful? React with 👍 / 👎.
| worker.start(timeout=timeout) | ||
| workers.append(worker) | ||
| except Exception: | ||
| stop_workers(workers) |
There was a problem hiding this comment.
Here the worker is added to workers only after worker.start(...) succeeds. If startup raises after spawning the subprocess (for example health-check timeout or early process crash), the except path only stops previously appended workers and misses the in-flight one, leaking a worker process (and its reserved GPU/ports) into later tests.
Useful? React with 👍 / 👎.
test_igw_multiple_workers starts both HTTP and gRPC workers simultaneously, each needing its own GPU. Move to a separate TestIGWMultiWorker class with gpu(2) marker and gpu_offset=1 so the gRPC worker uses GPU 1. - Remove -k test_pd_mmlu filter from e2e-2gpu-pd so it collects all gpu(2) router tests including the moved test - Add if: !cancelled() to e2e-2gpu-pd so it runs even when e2e-1gpu-gateway has flaky failures Signed-off-by: Simon Lin <simon@lightseek.ai> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
d40870d to
85f97d0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85f97d0f3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| model_path = MODEL_SPECS.get("embedding", {}).get("model") | ||
| if model_path is None: | ||
| pytest.skip("Embedding model not found in MODEL_SPECS") |
There was a problem hiding this comment.
hf_reference_embeddings still reads MODEL_SPECS.get("embedding"), but this commit renamed that spec entry to "intfloat/e5-mistral-7b-instruct" in e2e_test/infra/model_specs.py; as a result model_path is always None and the fixture unconditionally calls pytest.skip, so the embedding correctness suite is silently skipped in every E2E embeddings run instead of validating numerical correctness.
Useful? React with 👍 / 👎.
1 participant
Summary
Remove all threading, locking, and parallel execution support from E2E tests, then flatten architectural patterns that only existed to support concurrency. Also cleans up fragmented logging configuration.
Closes #635 follow-up (sequential execution was merged, this removes the now-dead parallel infrastructure).
What changed
14 files modified, -547 net lines.
Phase 1 — Surface removal
pyproject.toml: Removepytest-parallelandpydependenciespr-test-rust.yml: Removeparallel_optsfrom CI workflowfixtures/hooks.py: Removeparallel_worker_count(),parallel_safe_log(), parallel session hooksfixtures/__init__.py: Remove parallel-related exportsfixtures/pool.py: RemoveModelInstanceref counting (acquire()/release()/_ref_count/_ref_lock)conftest.py: Remove per-thread logging, parallel docsPhase 2 — Deep lock removal
infra/model_pool.py: RemoveModelPool._lock(threading.RLock) and all 5with self._lock:blocksinfra/gpu_allocator.py: RemoveGPUAllocator._lockand all lock-protected methodsfixtures/setup_backend.py: Remove_release_workers(), all.acquire()/.release()callsfixtures/pool.py,router/test_worker_api.py,responses/test_builtin_tools.py,bindings_go/conftest.py: Remove all.acquire()/.release()callschat_completions/test_validation.py: Remove_tokenizer_lockembeddings/test_correctness.py: Remove_hf_embeddings_lockimport threadingPhase 3 — Architectural simplification
gpu_allocator.py: Merge_allocate_slots_unlockedintoallocate_slotsmodel_pool.py: Merge_startup_unlocked→startup,_get_unlocked→get(),_launch_workers_unlocked→launch_workersmodel_pool.py: Add optionalmodeparameter toget_workers_by_type(), simplifying all 4 callers from 2-line get+filter to 1-line callmodel_pool.py: Removelast_usedside effect fromget_workers_by_type()(query methods shouldn't affect eviction ordering)_get_unlockedreferencesLogging cleanup
conftest.py: Replace three per-package logger configs ("e2e_test","infra","fixtures") with single root logger — test modules likechat_completions.*,router.*,embeddings.*were previously uncovered by the named loggersWhy
Since merging #635 (sequential E2E execution), all threading infrastructure is dead code:
_unlockedmethod pairs are trivial passthroughsRemoving this code makes the E2E infra significantly easier to understand and modify.
How
_unlockedmerge pattern: Inline private method body into public method. Where_unlockedreturnedNoneto signal "release lock, retry", replaced withcontinueto the retry loop.get_workers_by_typemode param: Added optionalmode: ConnectionMode | Noneparameter with filter in the list comprehension. All 4 callers (3 insetup_backend.py, 1 inbindings_go/conftest.py) simplified.StreamHandler(sys.stdout)replaces per-package configuration. External library loggers still suppressed to WARNING.Test plan
ruff check .passesruff format --check .passesgrep -rn '_unlocked\|Caller must hold\|auto-acquires\|by other tests'returns no resultspytest --collect-onlysucceeds (test collection)Summary by CodeRabbit
Chores
Tests