Skip to content

Comments

[CI/Build] Move test_utils.py to tests/utils.py#4425

Merged
rkooo567 merged 9 commits intovllm-project:mainfrom
DarkLight1337:move-test-utils
May 13, 2024
Merged

[CI/Build] Move test_utils.py to tests/utils.py#4425
rkooo567 merged 9 commits intovllm-project:mainfrom
DarkLight1337:move-test-utils

Conversation

@DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Apr 28, 2024

Since #4335 was merged, I've noticed that the definition of ServerRunner in the tests is the same as in the test for OpenAI API. I have moved the class to the test utilities to avoid code duplication. (Although it only has been repeated twice so far, I will add another similar test suite in #4200 which would duplicate the code a third time)

Also, I have moved the test utilities file (test_utils.py) to under the test directory (tests/utils.py), since none of its code is actually used in the main package. Note that I have added __init__.py to each test subpackage and updated the ray.init() call in the test utilities file in order to relative import tests/utils.py.

@DarkLight1337 DarkLight1337 changed the title [CI/Build] Move test_utils to tests/utils [CI/Build] Move test_utils to tests/utils.py Apr 28, 2024
@DarkLight1337 DarkLight1337 changed the title [CI/Build] Move test_utils to tests/utils.py [CI/Build] Move test_utils.py to tests/utils.py Apr 28, 2024
@DarkLight1337
Copy link
Member Author

I'm not that familiar with how the nccl library is handled in vLLM. How would using relative imports cause the library to not be loaded?

@DarkLight1337
Copy link
Member Author

I think it should be OK now.

@rkooo567 rkooo567 self-assigned this May 1, 2024
@DarkLight1337
Copy link
Member Author

@rkooo567 did you forget to merge this?

@rkooo567
Copy link
Collaborator

oops. sorry .can you resolve the merge conflict?

@DarkLight1337
Copy link
Member Author

oops. sorry .can you resolve the merge conflict?

See if this passes

@DarkLight1337
Copy link
Member Author

DarkLight1337 commented May 13, 2024

Apart from the AMD tests failing due to out of storage space and kernels-test-3 being canceled, the tests have passed. I think we can merge this now.

@rkooo567
Copy link
Collaborator

amd failure seems unrelated. cc @simon-mo I am merging this

@rkooo567 rkooo567 merged commit 350f9e1 into vllm-project:main May 13, 2024
@DarkLight1337 DarkLight1337 deleted the move-test-utils branch May 13, 2024 14:57
@youkaichao
Copy link
Member

It's a little bit late but I find pytorch also has some testing code in the main library https://github.com/pytorch/pytorch/tree/main/torch/testing . Maybe we need to discuss it. Relative import used in this PR is quite fragile .

@rkooo567
Copy link
Collaborator

Hmm is there a way to just exclude it in actual wheel?

@youkaichao
Copy link
Member

Hmm is there a way to just exclude it in actual wheel?

Not sure. But why do we care about this anyway? It's just a single python file with dozens of lines.

@DarkLight1337
Copy link
Member Author

DarkLight1337 commented May 14, 2024

Relative import used in this PR is quite fragile.

By this, do you mean that it breaks calling pytest from inner directories? I think this is not necessarily tied to the location of utils. The main reason why I moved it to tests directory is to avoid accidentally making test-only dependencies required for the user, rather than reducing wheel size.

To reinstate this functionality while keeping utilities in the tests directory, we can import the utilities by import tests.utils rather than the original import vllm.testing_utils. Prior to this PR, some test code already referred to the root conftest in a similar manner (import tests.conftest), so it should work.

robertgshaw2-redhat pushed a commit to neuralmagic/nm-vllm that referenced this pull request May 19, 2024
Since vllm-project#4335 was merged, I've noticed that the definition of ServerRunner in the tests is the same as in the test for OpenAI API. I have moved the class to the test utilities to avoid code duplication. (Although it only has been repeated twice so far, I will add another similar test suite in vllm-project#4200 which would duplicate the code a third time)

Also, I have moved the test utilities file (test_utils.py) to under the test directory (tests/utils.py), since none of its code is actually used in the main package. Note that I have added __init__.py to each test subpackage and updated the ray.init() call in the test utilities file in order to relative import tests/utils.py.
dtrifiro pushed a commit to dtrifiro/vllm that referenced this pull request May 21, 2024
Since vllm-project#4335 was merged, I've noticed that the definition of ServerRunner in the tests is the same as in the test for OpenAI API. I have moved the class to the test utilities to avoid code duplication. (Although it only has been repeated twice so far, I will add another similar test suite in vllm-project#4200 which would duplicate the code a third time)

Also, I have moved the test utilities file (test_utils.py) to under the test directory (tests/utils.py), since none of its code is actually used in the main package. Note that I have added __init__.py to each test subpackage and updated the ray.init() call in the test utilities file in order to relative import tests/utils.py.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants