-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: add TGIS CLI commands #92
Conversation
Hi @rafvasq. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Looks good @rafvasq ! Once we sync with upstream, we can get this in! |
/retest |
ab66c57
to
a38524f
Compare
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com> Co-authored-by: Prashant Gupta <prashantgupta@us.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need changes to this file anymore, the __main__.py
file handles starting both servers now!
return to_remove | ||
|
||
|
||
def convert_file(pt_file: Path, sf_file: Path, discard_names: List[str]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth checking if this function is the same thing? - https://github.com/vllm-project/vllm/blob/main/vllm/model_executor/model_loader/weight_utils.py#L81
vllm/scripts.py
Outdated
@@ -82,6 +96,151 @@ def chat(system_prompt: Optional[str], model_name: str, | |||
print(output) | |||
|
|||
|
|||
def download_weights( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if moving all these functions to a file that's not going to be changed by upstream vllm will help in future merge conflicts (Basically keeping our changes to a minimum to files that might be changed in upstream)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't all of this go to https://github.com/opendatahub-io/vllm-tgis-adapter ? @prashantgupta24
@dtrifiro I had debated that, but yeah that might make the future upstream conflict issue go away if we do that. Might need some changes to make it work with the adapter |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rafvasq The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@rafvasq can you try incorporating this in the vllm-tgis-adapter that Daniele linked above? Apologies for moving this PR around so much, but a lot of things are changing rapidly |
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
No problem @prashantgupta24! |
Closing as this belongs in |
* Reading the shapes csv only once and writing only if a new shape is deicovered * fix lint --------- Co-authored-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
Related to
vllm-project#5090
Previously, vllm-project#4167