[DRAFT] Add vllm backend for ilab serve - #1276
Conversation
Signed-off-by: Ali Maredia <amaredia@redhat.com>
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| -r requirements.txt | ||
|
|
||
| # newer vllm does not serve Merlinite | ||
| vllm<0.4.3 |
There was a problem hiding this comment.
Instead of another requirements file, how about you use an optional dependency?
Add this to pyproject.toml
[project.optional-dependencies]
vllm = ["vllm<0.4.3"]then you can install vllm support with pip install instructlab[vllm]
| rouge-score>=0.1.2,<0.2.0 | ||
| sentencepiece>=0.2.0,<0.3.0 | ||
| tokenizers>=0.15.2,<0.16.0 | ||
| tokenizers>=0.19.1,<0.20.0 |
There was a problem hiding this comment.
This break Intel Gaudi support. Is the version bump really required? Could you just add the higher version to the optional dependencies instead of main dependencies?
| torch>=2.2.1,<3.0.0 ; python_version != '3.10' | ||
| tqdm>=4.66.2,<5.0.0 | ||
| transformers>=4.30.0,<=4.38.2 | ||
| transformers>=4.41.2,<5.0.0 |
There was a problem hiding this comment.
Same problem, this version bump also breaks Intel Gaudi support.
The optimum packages has transformers <4.39.0,>=4.38.0 ; extra == 'habana'
There was a problem hiding this comment.
I have filed huggingface/optimum#1895 with optimum and asked to lift the upper version bound of transformers. This will also allow us to bump the tokenizers version.
There was a problem hiding this comment.
I got feedback on my feature request. The next versions of optimum and optimum-habana will support transformers >= 4.40.0, < 4.41.0. In the mean time, we have to support >=4.38.2.
| # pylint: disable=C0415 | ||
| # Local | ||
| from .server import ServerException, server | ||
| from pathlib import PurePath |
There was a problem hiding this comment.
Please move this to top-level import. Also why are you using PurePath instead of Path here? There is rarely a need to use PurePath except for special circumstances. See top level module docs of https://docs.python.org/3/library/pathlib.html
| host, | ||
| port, | ||
| if use_vllm or serve_safetensors: | ||
| import importlib.util |
There was a problem hiding this comment.
Move this to top-level import
| vllm_cmd = ["python", "-m", "vllm.entrypoints.openai.api_server", "--model", model_path] | ||
|
|
||
| if vllm_chat_template is not None: | ||
| vllm_cmd = vllm_cmd + vllm_chat_template.split() |
There was a problem hiding this comment.
| vllm_cmd = vllm_cmd + vllm_chat_template.split() | |
| vllm_cmd.extend(vllm_chat_template.split()) |
| vllm_cmd = vllm_cmd + vllm_chat_template.split() | ||
|
|
||
| if vllm_args is not None: | ||
| vllm_cmd = vllm_cmd + vllm_args.split() |
There was a problem hiding this comment.
| vllm_cmd = vllm_cmd + vllm_args.split() | |
| vllm_cmd.extend(vllm_args.split()) |
|
|
||
| vllm_cmd_str = " ".join(vllm_cmd) | ||
| ctx.obj.logger.info(f"vllm serving command is: {vllm_cmd_str}") | ||
| subprocess.run(args=vllm_cmd) |
There was a problem hiding this comment.
You either need to check the status of the return object or use some function like subprocess.check_call to check whether the call fails.
| ctx.obj.logger.info(f"vllm serving command is: {vllm_cmd_str}") | ||
| subprocess.run(args=vllm_cmd) | ||
| else: | ||
| # pylint: disable=C0415 |
There was a problem hiding this comment.
Please use human-readable short names instead of number constants. They are easier to recognize.
| # pylint: disable=C0415 | |
| # pylint: disable=import-outside-toplevel |
|
|
||
| ctx.obj.logger.info(f"Serving backend is llama-cpp") | ||
| ctx.obj.logger.info( | ||
| f"Using model '{model_path}' with {llama_cpp_gpu_layers} gpu-layers and {llama_cpp_max_ctx_size} max context size." |
There was a problem hiding this comment.
It's considered bad practice to use f-strings with logging. You should use logging's string interpolation instead.
leseb
left a comment
There was a problem hiding this comment.
Thanks for the initial shot, how about we freeze this for a moment and validate the design first? I'd like to avoid back and forth between the design change and the current code.
|
closed for #1386 |
Changes
Which issue is resolved by this Pull Request:
Resolves #1106
Description of your changes:
Initial support serving models with vllm as a backend. Further discussion on design is being tracked at #1106.