[CI][AMD][BugFix][P/D] Add default_vllm_config to test_moriio_connector.py so tests pass#33739
Conversation
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
There was a problem hiding this comment.
Code Review
This pull request aims to fix two failing tests by adding the default_vllm_config fixture. While this resolves the initial AssertionError, it likely introduces a more subtle issue. The default_vllm_config fixture provides a default VllmConfig where model_config is None, but the code path exercised by these tests requires a model_config to be set. This could lead to an AttributeError during test execution.
I've provided comments on both test functions suggesting a more robust fix: using set_current_vllm_config with the vllm_config object created within each test. This will ensure the correct configuration is used throughout the test's execution.
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
|
Can you show the full output of the failing test? (including the failed assertion callstack) |
Here you go @orozery. @tjtanaa is OOO from what I understand. |
@orozery The indentation makes it looks like I'm doing much more, especially since pre-commit had to reformat. It's really more like a two-line change. |
|
Thanks! got it now. Can you reduce the change to nest just this line under the |
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
Done, still works! |
|
You can still de-indent all other lines, so you would get ~4 lines changed. |
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
…o rasmith_fix_moriio_test
I did, except I had to keep lines 439-441 under the with statement otherwise the test fails. |
| connector.connector_worker = FakeMorIIOConnectorWorker( | ||
| vllm_config, connector.engine_id, hand_shake_latency=0 | ||
| ) | ||
| with set_current_vllm_config(vllm_config): |
There was a problem hiding this comment.
Thanks for the fix, LGTM.
Issue introduced by #30531 and #29575.
Noted related PR we can refer to : #31747, nixl also uses a similar approach
line: https://github.com/vllm-project/vllm/pull/31747/changes#diff-6ec47a6ca238ea4d626f7c5a87ab146a486dc61d5c4b024ef494622b5bf33636R312
…or.py so tests pass (vllm-project#33739) Signed-off-by: Randall Smith <Randall.Smith@amd.com>
…or.py so tests pass (vllm-project#33739) Signed-off-by: Randall Smith <Randall.Smith@amd.com>
…or.py so tests pass (vllm-project#33739) Signed-off-by: Randall Smith <Randall.Smith@amd.com>
…or.py so tests pass (vllm-project#33739) Signed-off-by: Randall Smith <Randall.Smith@amd.com>
…or.py so tests pass (vllm-project#33739) Signed-off-by: Randall Smith <Randall.Smith@amd.com> Signed-off-by: Andrii Skliar <askliar@nvidia.com>
…or.py so tests pass (vllm-project#33739) Signed-off-by: Randall Smith <Randall.Smith@amd.com>
Purpose
test_moriio_handshake_returns_metadataandtest_register_kv_cachesdo not make use of theset_vllm_configfixture and both result in this error when being ran:Test Plan
pytest -sv test_moriio_connector.pyTest Result
5 passed, 3 warnings in 5.72sEssential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.