Repository navigation
refactor(e2e): simplify test infrastructure by removing duplication and dead code - #587
Merged
Merged
Conversation
…nd dead code What changed: - e2e_test/fixtures/ports.py: deleted (dead code, zero importers) - e2e_test/fixtures/__init__.py: removed legacy module docstring referencing deleted files - e2e_test/conftest.py: removed duplicate logger assignment - e2e_test/fixtures/setup_backend.py: removed unused `request` param from _setup_pd_http_backend and _setup_pd_grpc_backend; extracted _release_workers helper replacing 11 inline try/except release loops - e2e_test/fixtures/pool.py: extracted _get_model_instance helper deduplicating marker extraction logic shared by model_client and model_base_url fixtures - e2e_test/infra/gpu_allocator.py: hoisted loop-invariant is_free function and threshold_desc out of the polling loop in wait_for_gpu_memory_to_clear - e2e_test/infra/process_utils.py: wrapped wait_for_workers_ready polling in requests.Session for connection reuse (matching wait_for_health pattern) - e2e_test/bindings_go/conftest.py: replaced hand-rolled _find_free_port, _wait_for_server, and process termination with infra utilities (get_open_port, wait_for_health, terminate_process) Why: The e2e_test infrastructure had accumulated dead code (ports.py), duplicated patterns (worker release loops, model instance extraction, port finding), and minor inefficiencies (loop-invariant function defs, missing connection reuse). How: Identified issues via code reuse, quality, and efficiency review. Applied targeted extractions and replacements preserving all existing behavior. Net result: -85 lines across 8 files. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
requested review from
CatherineSue,
XinyueZhang369 and
key4ng
as code owners
March 3, 2026 14:55
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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:
📝 WalkthroughWalkthroughReplaces ad-hoc test infra utilities with shared infra helpers, removes a deprecated ports module and a module-level logger, centralizes model-pool acquisition and worker-release logic, and optimizes GPU and process helper loops across e2e test fixtures and infra modules. Changes
Sequence Diagram(s)(omitted — changes are refactors/cleanup without new multi-component control flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/bindings_go/conftest.py`:
- Around line 225-226: get_open_port() currently reserves ports but the fixtures
never release them; update each fixture that assigns oai_port (and the similar
assignments at the other occurrences) to hold the reservation handle returned by
get_open_port() and release it in the fixture teardown/teardown_factory (e.g.,
call the reservation's release/close method or use the provided context manager)
so the reserved port is freed after the server stops; reference the
get_open_port() calls in the fixtures and add corresponding release/close logic
in their teardown code paths.
In `@e2e_test/fixtures/pool.py`:
- Around line 173-199: The helper _get_model_instance is calling ModelPool.get
with only model_id but ModelPool.get requires a mode parameter; update the call
in _get_model_instance to pass the correct mode (e.g., the desired enum/string
used by your pool) so ModelPool.get(model_id, mode) is invoked; ensure the mode
value matches how model_client and model_base_url fixtures expect to acquire
instances (refer to the ModelPool.get signature and the fixtures that call
_get_model_instance to choose the correct mode).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
e2e_test/bindings_go/conftest.pye2e_test/conftest.pye2e_test/fixtures/__init__.pye2e_test/fixtures/pool.pye2e_test/fixtures/ports.pye2e_test/fixtures/setup_backend.pye2e_test/infra/gpu_allocator.pye2e_test/infra/process_utils.py
💤 Files with no reviewable changes (3)
- e2e_test/fixtures/init.py
- e2e_test/fixtures/ports.py
- e2e_test/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e_test/fixtures/pool.py (1)
186-199:⚠️ Potential issue | 🔴 CriticalPass
modetoModelPool.get()to avoid runtime failure.At Line 198,
_get_model_instancecallsmodel_pool.get(model_id)withoutmode. IfModelPool.getrequires it, this breaks bothmodel_clientandmodel_base_urlfixture setup paths.Suggested fix
def _get_model_instance( request: pytest.FixtureRequest, model_pool: ModelPool, fixture_name: str ) -> ModelInstance: @@ - from infra import PARAM_MODEL + from infra import ConnectionMode, PARAM_MODEL @@ try: - return model_pool.get(model_id) + return model_pool.get(model_id, ConnectionMode.HTTP) except KeyError: pytest.skip(f"Model {model_id} not available in model pool")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/pool.py` around lines 186 - 199, The call to model_pool.get in _get_model_instance omits the required mode argument; update the call to model_pool.get(model_id, mode=mode) and ensure you obtain mode (e.g., from the test marker via marker.kwargs.get('mode') or from the surrounding fixture parameter named mode) before calling get so ModelPool.get receives the correct mode.
🤖 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 188-196: The code currently assumes marker.args contains an
element and indexing marker.args[0] can raise IndexError for malformed markers
(e.g., `@pytest.mark.model`()); update the marker handling after
request.node.get_closest_marker(PARAM_MODEL) to verify marker.args is non-empty
and call pytest.fail with a clear message if not, then safely assign model_id =
marker.args[0]; reference the marker lookup (request.node.get_closest_marker),
the constant PARAM_MODEL, the fixture_name used in the failure message, and the
model_id assignment to locate and update the logic.
---
Duplicate comments:
In `@e2e_test/fixtures/pool.py`:
- Around line 186-199: The call to model_pool.get in _get_model_instance omits
the required mode argument; update the call to model_pool.get(model_id,
mode=mode) and ensure you obtain mode (e.g., from the test marker via
marker.kwargs.get('mode') or from the surrounding fixture parameter named mode)
before calling get so ModelPool.get receives the correct mode.
slin1237
force-pushed
the
slin/e2e-refactor
branch
from
March 3, 2026 21:38
2ecb2b4 to
16415bc
Compare
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)
256-263:⚠️ Potential issue | 🟡 MinorHandle
communicate(timeout=5)timeout in failure paths.If
process.communicate(timeout=5)times out,subprocess.TimeoutExpiredis raised and masks the intendedpytest.fail(...)message, losing diagnostics about why the Go OAI server failed to start.💡 Proposed fix
except TimeoutError: - stdout, stderr = process.communicate(timeout=5) + try: + stdout, stderr = process.communicate(timeout=5) + except subprocess.TimeoutExpired: + stdout = b"<stdout unavailable: process did not exit in 5s>" + stderr = b"<stderr unavailable: process did not exit in 5s>" pytest.fail( f"Go OAI server failed to start.\n" f"Command: {' '.join(cmd)}\n" f"stdout: {stdout.decode()}\n" f"stderr: {stderr.decode()}" )Applies to lines 257 and 343.
🤖 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 256 - 263, The failure path currently calls process.communicate(timeout=5) inside the except TimeoutError block which can raise subprocess.TimeoutExpired and hide the intended pytest.fail diagnostics; update the except block so you catch subprocess.TimeoutExpired around process.communicate, and in that case kill/terminate the process (process.kill() or process.terminate()) and call process.communicate() without a timeout to collect stdout/stderr, then call pytest.fail(...) with the collected output; reference the existing symbols process.communicate, subprocess.TimeoutExpired, process.kill()/process.terminate, and pytest.fail to locate where to add the try/except and cleanup logic.
♻️ Duplicate comments (1)
e2e_test/bindings_go/conftest.py (1)
270-271:⚠️ Potential issue | 🟠 MajorEnsure port release runs even if process termination fails.
release_port(oai_port)is currently skipped ifterminate_process(...)raises, which can reintroduce reserved-port leaks in flaky teardown paths.💡 Proposed fix
finally: logger.info("Shutting down Go OAI server...") - terminate_process(process, timeout=10) - release_port(oai_port) + try: + terminate_process(process, timeout=10) + finally: + release_port(oai_port) @@ finally: logger.info("Shutting down Go OAI server...") - terminate_process(process, timeout=10) - release_port(oai_port) + try: + terminate_process(process, timeout=10) + finally: + release_port(oai_port)Also applies to: 359-360
🤖 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 270 - 271, The teardown currently calls terminate_process(process, timeout=10) followed by release_port(oai_port) but if terminate_process raises the release_port call is skipped; wrap the terminate_process call in a try/finally (or use try/except/finally) so release_port(oai_port) is executed in the finally block regardless of errors; apply the same change to the other teardown location that also calls terminate_process(...) then release_port(...) to guarantee ports are always released.
🤖 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 216-225: After obtaining the pool instance via
_get_model_instance, ensure instance.release() runs in all cases by wrapping the
client creation/yield in a try/finally: acquire instance as now, then use try:
create the openai.OpenAI client and yield it; finally: call instance.release()
so the release happens on client-creation failure, yield exit, or exception;
keep references to _get_model_instance and instance.release to locate the
change.
- Around line 197-200: The current except KeyError branch in the model
acquisition code swallows a missing-model condition by calling pytest.skip;
instead fail fast so infra/config regressions surface. Replace the
pytest.skip(...) call with pytest.fail(...) (or raise a clear assertion) in the
except KeyError handler around the model_pool.get(model_id, ConnectionMode.HTTP)
call so that attempting to acquire a required model (model_id) triggers an
explicit test failure rather than a skip; update references to pytest.skip ->
pytest.fail and keep the original error message.
---
Outside diff comments:
In `@e2e_test/bindings_go/conftest.py`:
- Around line 256-263: The failure path currently calls
process.communicate(timeout=5) inside the except TimeoutError block which can
raise subprocess.TimeoutExpired and hide the intended pytest.fail diagnostics;
update the except block so you catch subprocess.TimeoutExpired around
process.communicate, and in that case kill/terminate the process (process.kill()
or process.terminate()) and call process.communicate() without a timeout to
collect stdout/stderr, then call pytest.fail(...) with the collected output;
reference the existing symbols process.communicate, subprocess.TimeoutExpired,
process.kill()/process.terminate, and pytest.fail to locate where to add the
try/except and cleanup logic.
---
Duplicate comments:
In `@e2e_test/bindings_go/conftest.py`:
- Around line 270-271: The teardown currently calls terminate_process(process,
timeout=10) followed by release_port(oai_port) but if terminate_process raises
the release_port call is skipped; wrap the terminate_process call in a
try/finally (or use try/except/finally) so release_port(oai_port) is executed in
the finally block regardless of errors; apply the same change to the other
teardown location that also calls terminate_process(...) then release_port(...)
to guarantee ports are always released.
Comment on lines
197
to
200
| try: | ||
| # get() auto-acquires the returned instance | ||
| instance = model_pool.get(model_id) | ||
| return model_pool.get(model_id, ConnectionMode.HTTP) | ||
| except KeyError: | ||
| pytest.skip(f"Model {model_id} not available in model pool") |
There was a problem hiding this comment.
Prefer explicit failure over skip when required model acquisition fails.
At Line 200, pytest.skip(...) can hide infra/config regressions; this path should fail fast with a clear message.
💡 Proposed fix
try:
return model_pool.get(model_id, ConnectionMode.HTTP)
except KeyError:
- pytest.skip(f"Model {model_id} not available in model pool")
+ pytest.fail(
+ f"Model {model_id} not available in model pool for {fixture_name}; "
+ "check model prelaunch requirements and test markers."
+ )Based on learnings: tests should fail explicitly when required services are unavailable rather than being skipped to catch misconfigurations.
📝 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
| try: | |
| # get() auto-acquires the returned instance | |
| instance = model_pool.get(model_id) | |
| return model_pool.get(model_id, ConnectionMode.HTTP) | |
| except KeyError: | |
| pytest.skip(f"Model {model_id} not available in model pool") | |
| try: | |
| return model_pool.get(model_id, ConnectionMode.HTTP) | |
| except KeyError: | |
| pytest.fail( | |
| f"Model {model_id} not available in model pool for {fixture_name}; " | |
| "check model prelaunch requirements and test markers." | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/fixtures/pool.py` around lines 197 - 200, The current except
KeyError branch in the model acquisition code swallows a missing-model condition
by calling pytest.skip; instead fail fast so infra/config regressions surface.
Replace the pytest.skip(...) call with pytest.fail(...) (or raise a clear
assertion) in the except KeyError handler around the model_pool.get(model_id,
ConnectionMode.HTTP) call so that attempting to acquire a required model
(model_id) triggers an explicit test failure rather than a skip; update
references to pytest.skip -> pytest.fail and keep the original error message.
What changed: - e2e_test/fixtures/pool.py: added ModelInstance to the TYPE_CHECKING import block to fix ruff F821 (undefined name) for the _get_model_instance return type annotation Why: The extracted _get_model_instance helper used ModelInstance in its return type but the import was missing — previously the type was only used inline within fixtures that imported it locally. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/e2e-refactor
branch
from
March 3, 2026 22:23
16415bc to
925cdda
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
e2e_test/fixtures/pool.py (2)
197-200:⚠️ Potential issue | 🟠 MajorPrefer explicit failure over skip when required model acquisition fails.
Using
pytest.skipat Line 200 can hide infrastructure or configuration regressions. When a test explicitly requires a model via@pytest.mark.model(), its absence indicates a misconfiguration that should fail fast rather than silently skip.Proposed fix
try: return model_pool.get(model_id, ConnectionMode.HTTP) except KeyError: - pytest.skip(f"Model {model_id} not available in model pool") + pytest.fail( + f"Model {model_id} not available in model pool for {fixture_name}; " + "check model prelaunch requirements and test markers." + )Based on learnings: "tests should fail explicitly when the required MCP server is unavailable rather than being skipped... The team prefers explicit failures over silent skips to catch misconfigurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/pool.py` around lines 197 - 200, The code currently swallows a KeyError from model_pool.get(model_id, ConnectionMode.HTTP) by calling pytest.skip, which hides misconfiguration; instead, catch the KeyError and fail the test explicitly (e.g., call pytest.fail(...) or raise a RuntimeError) with a clear message that includes model_id and context (attempting to acquire from model_pool via model_pool.get with ConnectionMode.HTTP) so missing required models cause a hard test failure.
216-225:⚠️ Potential issue | 🟠 MajorEnsure
instance.release()always runs even if client creation fails.If
openai.OpenAI()raises an exception after the instance is acquired at Line 216, the release at Line 225 is never reached, leaking the pool reference.Proposed fix
instance = _get_model_instance(request, model_pool, "model_client") - client = openai.OpenAI( - base_url=f"{instance.base_url}/v1", - api_key="not-used", - ) - - yield client - - instance.release() + try: + client = openai.OpenAI( + base_url=f"{instance.base_url}/v1", + api_key="not-used", + ) + yield client + finally: + instance.release()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/pool.py` around lines 216 - 225, The code acquires an instance via _get_model_instance(...) then constructs openai.OpenAI(...) but calls instance.release() only after yield, so if openai.OpenAI raises the release is never called; wrap the client construction and yield in a try/finally (or convert to a contextmanager) so that instance.release() is invoked in the finally block regardless of whether openai.OpenAI(...) or subsequent code raises; keep the same variables (instance, client) and ensure the generator yields the client inside the try and always calls instance.release() in finally.
🤖 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/bindings_go/conftest.py`:
- Around line 254-257: The TimeoutError handler after wait_for_health should
guard against subprocess.TimeoutExpired when calling process.communicate: catch
subprocess.TimeoutExpired around the process.communicate(timeout=5) call (in the
block handling wait_for_health timeouts), capture whatever partial stdout/stderr
is available or note that communicate timed out, and then call pytest.fail with
a diagnostic that includes the stdout/stderr or a clear message that communicate
timed out; update both occurrences that wrap process.communicate (referencing
wait_for_health, process.communicate, TimeoutError, subprocess.TimeoutExpired,
and pytest.fail) so the test fails with useful diagnostics instead of raising
TimeoutExpired.
- Around line 225-226: The port reservation done by get_open_port() can leak if
subprocess.Popen(...) raises before entering the existing try/finally; wrap the
spawn in a try/finally so release_port(port) always runs on failure: call
get_open_port() to get oai_port (and the other reserved port at lines ~299-300),
then immediately enter try, attempt subprocess.Popen(...) inside that try, and
in the finally call release_port(oai_port) (and the corresponding release for
the other reserved port) when the process object was not successfully created or
when cleanup is needed; alternatively move subprocess.Popen(...) inside the
existing try block so that release_port(...) is guaranteed to run in the finally
for both get_open_port()/oai_port and the other reserved port.
In `@e2e_test/fixtures/pool.py`:
- Around line 239-243: Wrap the yield and release in a try/finally to guarantee
cleanup: after obtaining the instance via _get_model_instance(request,
model_pool, "model_base_url"), yield instance.base_url inside the try block and
call instance.release() in the finally block so release() always runs even if
the test errors.
---
Duplicate comments:
In `@e2e_test/fixtures/pool.py`:
- Around line 197-200: The code currently swallows a KeyError from
model_pool.get(model_id, ConnectionMode.HTTP) by calling pytest.skip, which
hides misconfiguration; instead, catch the KeyError and fail the test explicitly
(e.g., call pytest.fail(...) or raise a RuntimeError) with a clear message that
includes model_id and context (attempting to acquire from model_pool via
model_pool.get with ConnectionMode.HTTP) so missing required models cause a hard
test failure.
- Around line 216-225: The code acquires an instance via
_get_model_instance(...) then constructs openai.OpenAI(...) but calls
instance.release() only after yield, so if openai.OpenAI raises the release is
never called; wrap the client construction and yield in a try/finally (or
convert to a contextmanager) so that instance.release() is invoked in the
finally block regardless of whether openai.OpenAI(...) or subsequent code
raises; keep the same variables (instance, client) and ensure the generator
yields the client inside the try and always calls instance.release() in finally.
Comment on lines
+225
to
226
| oai_port = get_open_port() | ||
|
|
There was a problem hiding this comment.
Ensure reserved ports are released even when process spawn fails.
At Line 225 and Line 299, get_open_port() reserves immediately, but subprocess.Popen(...) is executed before entering try/finally. If spawn fails, release_port() is skipped and reservations leak.
💡 Proposed fix
@@ def go_oai_server(...):
- process = subprocess.Popen(
- cmd,
- env=env,
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
-
- try:
+ process: subprocess.Popen | None = None
+ try:
+ process = subprocess.Popen(
+ cmd,
+ env=env,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE,
+ )
@@
finally:
logger.info("Shutting down Go OAI server...")
- terminate_process(process, timeout=10)
+ if process is not None:
+ terminate_process(process, timeout=10)
release_port(oai_port)
@@ def go_oai_server_multi(...):
- process = subprocess.Popen(
- cmd,
- env=env,
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
-
- try:
+ process: subprocess.Popen | None = None
+ try:
+ process = subprocess.Popen(
+ cmd,
+ env=env,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE,
+ )
@@
finally:
logger.info("Shutting down Go OAI server...")
- terminate_process(process, timeout=10)
+ if process is not None:
+ terminate_process(process, timeout=10)
release_port(oai_port)Also applies to: 299-300
🤖 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 225 - 226, The port
reservation done by get_open_port() can leak if subprocess.Popen(...) raises
before entering the existing try/finally; wrap the spawn in a try/finally so
release_port(port) always runs on failure: call get_open_port() to get oai_port
(and the other reserved port at lines ~299-300), then immediately enter try,
attempt subprocess.Popen(...) inside that try, and in the finally call
release_port(oai_port) (and the corresponding release for the other reserved
port) when the process object was not successfully created or when cleanup is
needed; alternatively move subprocess.Popen(...) inside the existing try block
so that release_port(...) is guaranteed to run in the finally for both
get_open_port()/oai_port and the other reserved port.
- Move subprocess.Popen inside try/finally in go_oai_server and go_oai_server_multi so release_port() runs even if spawn fails - Handle subprocess.TimeoutExpired in health-check diagnostics to avoid losing the pytest.fail message when process.communicate hangs - Wrap yield/release in try/finally for model_client and model_base_url fixtures to guarantee instance.release() on test exceptions Addresses CodeRabbit review feedback on PR #587. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
e2e_test/fixtures/pool.py (2)
197-200:⚠️ Potential issue | 🟠 MajorUse
pytest.failinstead ofpytest.skipwhen required model acquisition fails.This fixture is a required test dependency path; skipping here can hide pool/config regressions.
Based on learnings: tests should fail explicitly when required services are unavailable rather than being skipped to catch misconfigurations.💡 Proposed fix
try: return model_pool.get(model_id, ConnectionMode.HTTP) except KeyError: - pytest.skip(f"Model {model_id} not available in model pool") + pytest.fail( + f"Model {model_id} not available in model pool for {fixture_name}; " + "check model prelaunch requirements and test markers." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/pool.py` around lines 197 - 200, The fixture currently swallows a missing model by calling pytest.skip in the except KeyError; change this to explicitly fail the test using pytest.fail so missing required models cause test failures. Locate the try/except around model_pool.get(model_id, ConnectionMode.HTTP) in the fixture and replace the pytest.skip(...) call with pytest.fail(f"Model {model_id} not available in model pool") (or equivalent) so acquisition failures in model_pool.get raise explicit test failures instead of skipping.
218-226:⚠️ Potential issue | 🟠 MajorWrap client construction in the
try/finallythat releases the model instance.If
openai.OpenAI(...)throws,instance.release()is skipped in the current structure.💡 Proposed fix
- client = openai.OpenAI( - base_url=f"{instance.base_url}/v1", - api_key="not-used", - ) - try: + client = openai.OpenAI( + base_url=f"{instance.base_url}/v1", + api_key="not-used", + ) yield client finally: instance.release()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/pool.py` around lines 218 - 226, The client is constructed before the try/finally so if openai.OpenAI(...) raises, instance.release() is never called; move the openai.OpenAI(...) construction into the try block (e.g., set client = None before the try, then inside try assign client = openai.OpenAI(...), yield client) and keep instance.release() in finally so instance.release() always runs even when the constructor raises; do not swallow the original exception (let it propagate after finally).e2e_test/bindings_go/conftest.py (1)
245-279:⚠️ Potential issue | 🔴 CriticalInitialize
processbeforetryto avoid cleanup-timeUnboundLocalError.If
subprocess.Popen(...)fails,finallycallsterminate_process(process, ...)beforeprocessis bound. That can mask spawn failures and preventrelease_port(...)from executing.💡 Proposed fix
@@ - try: + process: subprocess.Popen | None = None + try: process = subprocess.Popen( cmd, env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) @@ finally: logger.info("Shutting down Go OAI server...") - terminate_process(process, timeout=10) + if process is not None: + terminate_process(process, timeout=10) release_port(oai_port) @@ - try: + process: subprocess.Popen | None = None + try: process = subprocess.Popen( cmd, env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) @@ finally: logger.info("Shutting down Go OAI server...") - terminate_process(process, timeout=10) + if process is not None: + terminate_process(process, timeout=10) release_port(oai_port)Also applies to: 338-374
🤖 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 245 - 279, Initialize process to None before the subprocess.Popen try block and guard the cleanup so terminate_process is only called if process is not None (and/or has been started), e.g., set process = None before calling subprocess.Popen(...) and in the finally block check the process variable before calling terminate_process(process, ...); apply the same change to the other similar spawn block that uses subprocess.Popen in this file so release_port still runs even if Popen fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e_test/bindings_go/conftest.py`:
- Around line 245-279: Initialize process to None before the subprocess.Popen
try block and guard the cleanup so terminate_process is only called if process
is not None (and/or has been started), e.g., set process = None before calling
subprocess.Popen(...) and in the finally block check the process variable before
calling terminate_process(process, ...); apply the same change to the other
similar spawn block that uses subprocess.Popen in this file so release_port
still runs even if Popen fails.
In `@e2e_test/fixtures/pool.py`:
- Around line 197-200: The fixture currently swallows a missing model by calling
pytest.skip in the except KeyError; change this to explicitly fail the test
using pytest.fail so missing required models cause test failures. Locate the
try/except around model_pool.get(model_id, ConnectionMode.HTTP) in the fixture
and replace the pytest.skip(...) call with pytest.fail(f"Model {model_id} not
available in model pool") (or equivalent) so acquisition failures in
model_pool.get raise explicit test failures instead of skipping.
- Around line 218-226: The client is constructed before the try/finally so if
openai.OpenAI(...) raises, instance.release() is never called; move the
openai.OpenAI(...) construction into the try block (e.g., set client = None
before the try, then inside try assign client = openai.OpenAI(...), yield
client) and keep instance.release() in finally so instance.release() always runs
even when the constructor raises; do not swallow the original exception (let it
propagate after finally).
This was referenced Mar 4, 2026
2 of 6 tasks
6 of 9 tasks
This was referenced May 13, 2026
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
1 participant
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.
Summary
Simplifies the E2E test infrastructure by removing dead code, extracting shared helpers, and replacing hand-rolled utilities with existing
inframodule functions. Net result: -85 lines across 8 files with no behavioral changes.What changed
e2e_test/fixtures/ports.pyinfra.get_open_port()e2e_test/fixtures/__init__.pye2e_test/conftest.pylogger = logging.getLogger(__name__)e2e_test/fixtures/setup_backend.pyrequestparam from_setup_pd_http_backend/_setup_pd_grpc_backend; extracted_release_workershelper replacing 11 inline try/except release loopse2e_test/fixtures/pool.py_get_model_instancehelper deduplicating marker extraction logic shared bymodel_clientandmodel_base_urlfixturese2e_test/infra/gpu_allocator.pyis_freefunction andthreshold_descout of polling loop inwait_for_gpu_memory_to_cleare2e_test/infra/process_utils.pywait_for_workers_readypolling inrequests.Sessionfor connection reuse (matchingwait_for_healthpattern)e2e_test/bindings_go/conftest.py_find_free_port,_wait_for_server, and process termination withinfrautilities (get_open_port,wait_for_health,terminate_process)Why
The E2E test infrastructure had accumulated dead code (
ports.py), duplicated patterns (worker release loops repeated 11 times, model instance extraction copied between fixtures, local port-finding reimplementing existing utility), and minor inefficiencies (function definitions recreated every loop iteration, missing HTTP connection reuse).How
Identified issues via parallel code reuse, quality, and efficiency review. Applied targeted extractions and replacements preserving all existing behavior. Each change is strictly refactoring — no new features or behavioral modifications.
Test plan
fixtures.ports(verified: zero importers)_release_workershelper covers all previous inline patternsSummary by CodeRabbit
Chores
Refactor