test(vllm): drop tests of APIs removed by PR #22 refactor - #43
Conversation
PR #22 commit 132066f ("Fix colocated mode weight sync") removed/inlined several private helpers on ``VLLMEngine`` and the module-level ``_response_json_or_fallback``, but the unit tests for them (added in PR #21) were not updated. They fail on the PR #22 HEAD with ``AttributeError``. Apply the same cleanup as commit 2ee402b ("Fix test", which removed the ``_skipped_if_not_leader`` tests) to the remaining drift: - Delete the tests for ``_weight_transfer_http_timeout`` (inlined into ``start_weight_update`` / ``finish_weight_update`` / ``_post_vllm_update_weights_http``). - Delete the tests for ``_response_json_or_fallback`` (the helper was removed; the inline replacement uses a simpler ``try: response.json()`` fallback that doesn't reproduce the wrapping behavior the tests asserted). - Delete ``test_http_base_requires_init`` (the explicit RuntimeError guard was removed; the test ``test_http_base_ipv6_host`` already covers the happy path). - In ``test_start_weight_update_posts_four_phase_endpoint`` and ``test_finish_weight_update_posts_empty_body``, drop the dead reference to ``_weight_transfer_http_timeout`` while keeping the endpoint/payload assertions, which still test real production behavior. Result: ``pytest tests/unit/backends/vllm_utils/test_vllm_engine.py`` goes from 8 failed / 18 passed to 20 passed.
There was a problem hiding this comment.
Code Review
This pull request removes several unit tests and assertions related to weight transfer HTTP timeouts and JSON response fallback handling in test_vllm_engine.py. The reviewer suggests that instead of completely removing the timeout assertions, the tests should verify that the default timeout of 900.0 seconds is correctly passed.
| @@ -83,7 +83,6 @@ def fake_post(endpoint: str, payload: dict, timeout: float): | |||
| assert len(calls) == 1 | |||
| assert calls[0][0] == "start_weight_update" | |||
| assert calls[0][1] == {"is_checkpoint_format": True} | |||
There was a problem hiding this comment.
Instead of completely removing the timeout assertion, we should verify that the default timeout of 900.0 seconds is correctly passed to the _post_json call.
| assert calls[0][1] == {"is_checkpoint_format": True} | |
| assert calls[0][1] == {"is_checkpoint_format": True} | |
| assert calls[0][2] == 900.0 |
| assert len(calls) == 1 | ||
| assert calls[0][0] == "finish_weight_update" | ||
| assert calls[0][1] == {} |
There was a problem hiding this comment.
Instead of dropping the timeout assertion, we should verify that the default timeout of 900.0 seconds is correctly passed to the _post_json call.
| assert len(calls) == 1 | |
| assert calls[0][0] == "finish_weight_update" | |
| assert calls[0][1] == {} | |
| assert len(calls) == 1 | |
| assert calls[0][0] == "finish_weight_update" | |
| assert calls[0][1] == {} | |
| assert calls[0][2] == 900.0 |
|
@aoshen02 Let's fix in on main branch |
|
PR #22 is now merged into main; reopening against main with broader scope to also restore the production behavior that the test drift was hiding (timeout-env precedence and response-parsing semantics) — see follow-up PR. |
Summary
Follow-up to @knlnguyen1802's request on #22 (comment) to send a small PR against
update_weights_tensorrather than a manual edit.Same kind of test/production drift as the one cleaned up in commit
2ee402b "Fix test", but for the other 8 unit tests that were also broken by the132066f "Fix colocated mode weight sync"refactor.On the current
update_weights_tensorHEAD,pytest tests/unit/backends/vllm_utils/test_vllm_engine.pyreturns 8 failed, 18 passed. After this PR: 20 passed.What broke
PR #22 commit
132066fremoved / inlined three things inslime/backends/vllm_utils/vllm_engine.py:VLLMEngine._weight_transfer_http_timeout()float(os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900"))instart_weight_update/finish_weight_update;_post_vllm_update_weights_httpuses a slightly different env-var precedence_response_json_or_fallback(response)try: return response.json() except Exception: return {"ok": True, "raw": response.text}_http_base()init-guard (RuntimeErrorwhenserver_hostunset)But PR #21 had added tests against those exact APIs in
tests/unit/backends/vllm_utils/test_vllm_engine.py, and commit2ee402bonly cleaned up the two_skipped_if_not_leadertests, leaving the other 8 drifting.What this PR does
Deletes 6 tests of fully-removed APIs:
test_weight_transfer_http_timeout_reads_envtest_weight_transfer_http_timeout_fallback_to_legacy_envtest_response_json_or_fallback_parses_dicttest_response_json_or_fallback_non_dict_wrappedtest_response_json_or_fallback_invalid_jsontest_http_base_requires_init(the happy path is still covered bytest_http_base_ipv6_host)Trims the dead
_weight_transfer_http_timeout()reference from 2 tests but keeps their endpoint/payload assertions, which still pin real production behavior:test_start_weight_update_posts_four_phase_endpointtest_finish_weight_update_posts_empty_bodyVerification
Diff is +3/-52 in a single file.