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

feat: Add model-util CLI #59

Merged
merged 27 commits into from
Aug 14, 2024
Merged

Conversation

rafvasq
Copy link
Contributor

@rafvasq rafvasq commented Jul 23, 2024

Changes:

  • Adds commands to the adapter using either entry point model-util or text-generation-server
    • model-util download-weights
    • model-util convert-to-safetensors
    • model-util convert-to-fast-tokenizer

Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Co-authored-by: Prashant Gupta <prashantgupta@us.ibm.com>
@rafvasq rafvasq marked this pull request as ready for review July 26, 2024 21:18
@dtrifiro
Copy link
Contributor

Please see the contributing guide for information on how to set up pre-commit so that CI does not fail

@dtrifiro
Copy link
Contributor

dtrifiro commented Aug 1, 2024

vllm@main CI will keep failing until #68 solves the problem and is merged

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 15.35088% with 193 lines in your changes missing coverage. Please review.

Project coverage is 54.70%. Comparing base (7cd77d1) to head (fbb0cd7).

Files Patch % Lines
src/vllm_tgis_adapter/tgis_utils/scripts.py 0.00% 104 Missing ⚠️
src/vllm_tgis_adapter/tgis_utils/hub.py 25.26% 71 Missing ⚠️
tests/test_hub.py 37.93% 18 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (7cd77d1) and HEAD (fbb0cd7). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (7cd77d1) HEAD (fbb0cd7)
6 3
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
- Coverage   62.08%   54.70%   -7.38%     
==========================================
  Files          21       24       +3     
  Lines        1216     1444     +228     
  Branches      213      259      +46     
==========================================
+ Hits          755      790      +35     
- Misses        386      579     +193     
  Partials       75       75              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@prashantgupta24
Copy link
Contributor

@rafvasq can you trim down the commits?

dtrifiro and others added 4 commits August 1, 2024 16:57
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.5.4 to 0.5.5.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md)
- [Commits](astral-sh/ruff@0.5.4...0.5.5)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
@rafvasq
Copy link
Contributor Author

rafvasq commented Aug 1, 2024

Trimmed them down, @prashantgupta24

@njhill
Copy link
Contributor

njhill commented Aug 2, 2024

Thanks @rafvasq, and thanks @prashantgupta24 @dtrifiro for all of the reviews. This is looking great!

I know there was already some discussion about this above but I'm wondering whether we should make the command something different. Maybe even just make it text-generation-server since these are mainly for backwards compatibility?

Or even something like model-util? Even better would I guess would be to have both, i.e. keep text-generation-server as an alias if that's possible, for backwards compatibility, but we can just document the new command.

@prashantgupta24
Copy link
Contributor

Thanks @rafvasq, and thanks @prashantgupta24 @dtrifiro for all of the reviews. This is looking great!

I know there was already some discussion about this above but I'm wondering whether we should make the command something different. Maybe even just make it text-generation-server since these are mainly for backwards compatibility?

Or even something like model-util? Even better would I guess would be to have both, i.e. keep text-generation-server as an alias if that's possible, for backwards compatibility, but we can just document the new command.

I was a bit hesitant in using text-generation-server just because it might confuse folks who have worked with TGIS earlier?

rafvasq added 2 commits August 2, 2024 09:42
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
@rafvasq rafvasq changed the title feat: Add TGIS CLI feat: Add model-util CLI Aug 2, 2024
@njhill
Copy link
Contributor

njhill commented Aug 5, 2024

Thanks @rafvasq, and thanks @prashantgupta24 @dtrifiro for all of the reviews. This is looking great!
I know there was already some discussion about this above but I'm wondering whether we should make the command something different. Maybe even just make it text-generation-server since these are mainly for backwards compatibility?
Or even something like model-util? Even better would I guess would be to have both, i.e. keep text-generation-server as an alias if that's possible, for backwards compatibility, but we can just document the new command.

I was a bit hesitant in using text-generation-server just because it might confuse folks who have worked with TGIS earlier?

Imho it shouldn't introduce confusion, just would hopefully allow their existing scripts/processes to work without change. But if it's not straightforward to make an alias then I guess not a huge deal.

@rafvasq
Copy link
Contributor Author

rafvasq commented Aug 12, 2024

@njhill, sorry I forgot to mention it but I did introduce the text-generation-server alias:

https://github.com/opendatahub-io/vllm-tgis-adapter/pull/59/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R50

@dtrifiro
Copy link
Contributor

Thanks @rafvasq. This looks very good at this point, just a few nits left

Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
@dtrifiro dtrifiro enabled auto-merge August 13, 2024 22:24
@dtrifiro dtrifiro removed the request for review from prashantgupta24 August 13, 2024 22:51
@dtrifiro dtrifiro added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 13, 2024
@prashantgupta24 prashantgupta24 added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 13, 2024
@prashantgupta24 prashantgupta24 added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 13, 2024
@prashantgupta24 prashantgupta24 added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 13, 2024
@prashantgupta24 prashantgupta24 added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 13, 2024
@prashantgupta24 prashantgupta24 added this pull request to the merge queue Aug 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 14, 2024
@dtrifiro dtrifiro merged commit 977103a into opendatahub-io:main Aug 14, 2024
3 checks passed
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.

5 participants