Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for python version 3.10 #194

Closed
wants to merge 1 commit into from

Conversation

varad-ahirwadkar
Copy link

This PR includes fixes to run the vllm-tgis-adapter using python 3.10

Description

For Python versions earlier than 3.11, StrEnum is not included in the standard library enum. Therefore, it is downloaded and imported separately.
The delete=False parameter in tempfile.TemporaryDirectory() is supported only from Python 3.12 onwards. As a workaround, mkdtemp() is used instead.

How Has This Been Tested?

Env:

Python: 3.10
vLLM: v0.6.4.post2

Running tests:

pytest --disable-pytest-warnings tests/ | tee output.txt
============================= test session starts ==============================
platform linux -- Python 3.10.13, pytest-8.3.4, pluggy-1.5.0
rootdir: /workspace/vllm-tgis-adapter
configfile: pyproject.toml
plugins: twisted-1.14.3, mock-3.14.0, asyncio-0.25.0, anyio-4.6.2.post1
asyncio: mode=strict, asyncio_default_fixture_loop_scope=None
collected 71 items / 6 deselected / 65 selected

tests/test_adapters.py ......                                            [  9%]
tests/test_grpc_server.py ....s..                                        [ 20%]
tests/test_http_server.py ...                                            [ 24%]
tests/test_init.py .                                                     [ 26%]
tests/test_peft_converter.py .                                           [ 27%]
tests/test_termination_log.py ...                                        [ 32%]
tests/test_tgis_utils.py ............................................    [100%]

=========================== short test summary info ============================
SKIPPED [1] tests/test_grpc_server.py:64: Lora is not available with this configuration
===== 64 passed, 1 skipped, 6 deselected, 19 warnings in 173.80s (0:02:53) =====

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@varad-ahirwadkar
Copy link
Author

@dtrifiro Could you please review this PR? Thanks

pyproject.toml Outdated Show resolved Hide resolved
@dtrifiro
Copy link
Contributor

@varad-ahirwadkar we don't really test this with anything but 3.12. Is updating to 3.12 a choice?

@varad-ahirwadkar
Copy link
Author

@varad-ahirwadkar we don't really test this with anything but 3.12. Is updating to 3.12 a choice?

Hi @dtrifiro,
Yes, will update to 3.12.
But are we considering support for Python 3.10 or 3.11?

@dtrifiro
Copy link
Contributor

We thought we'd be supporting multiple python versions at first, but the reality is that we're going to be tied to whatever we have in the https://github.com/opendatahub-io/vllm ubi Dockerfile(s) (which currently is 3.12). We currently also run CI only for 3.12

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.

2 participants