[CI/Build] Fix some V1 tests not being run #25569
Conversation
Signed-off-by: DarkLight1337 <[email protected]>
DarkLight1337
added
the
ready
![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 addresses an issue where some V1 tests were not being executed in the CI pipeline. The changes in .buildkite/test-pipeline.yaml correctly add test_kv_sharing.py and test_request.py to the test runs. The modifications in tests/v1/test_kv_sharing.py are intended to adapt the tests to a recent API change. However, I've identified a critical flaw in the updated test logic. The tests now include tautological assertions that don't validate the production code's behavior, which could provide a false sense of correctness. My review comments detail these issues and suggest how to resolve them.
Signed-off-by: DarkLight1337 <[email protected]>
sarckk
left a comment
sarckk
left a comment
There was a problem hiding this comment.
thanks for the catch!
note: since #22628 moved the actual KV sharing up logic to the gpu model runner, unit tests in
pytest tests/v1/worker/test_gpu_model_runner.py -k "test_init_kv_cache"
will cover the KV sharing logic including assertions that were removed in this PR.
NickLucche
left a comment
NickLucche
left a comment
There was a problem hiding this comment.
LGTM, left a comment about grouping these tests though
| - pytest -v -s v1/spec_decode | ||
| - pytest -v -s v1/kv_connector/unit | ||
| - pytest -v -s v1/metrics | ||
| - pytest -v -s v1/test_kv_sharing.py | ||
| - pytest -v -s v1/test_metrics_reader.py | ||
| - pytest -v -s v1/test_oracle.py | ||
| - pytest -v -s v1/test_request.py |
There was a problem hiding this comment.
I wonder whether we should factor these tests into their own folder so we can just do pytest -v v1/core ?
There was a problem hiding this comment.
Let's address that in another PR
There was a problem hiding this comment.
Is it possible to change this V1 Test others from running a list of files to running all files that are not covered by other tests?
There was a problem hiding this comment.
Technically I think it's possible - I can think of two ways to go about it:
- Scan the
test-pipeline.yamlfile and parse the commands corresponding to the rest of the V1 test CIs to figure out which tests are not being covered. - Add a pytest mark for the tests that are covered by the rest of the V1 test CIs , then we can exclude tests with those marks in the "other" CI
There was a problem hiding this comment.
But either way, it would be quite complicated
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: yewentao256 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
4 participants
Purpose
KV sharing tests have been failing since #22628 but have not been caught because the test isn't actually being run. This PR fixes the tests and also adds them to the CI.
I found that
test_request.pyis also not being run so I have added it to the CI as well.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.