Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Use new vllm/llama-cpp backends for evaluate#1539

Merged
mergify[bot] merged 2 commits intoinstructlab:maininstructlab/instructlab:mainfrom
danmcp:evalandvllmdanmcp/instructlab:evalandvllmCopy head branch name to clipboard
Jul 2, 2024
Merged

Use new vllm/llama-cpp backends for evaluate#1539
mergify[bot] merged 2 commits intoinstructlab:maininstructlab/instructlab:mainfrom
danmcp:evalandvllmdanmcp/instructlab:evalandvllmCopy head branch name to clipboard

Conversation

@danmcp
Copy link
Contributor

@danmcp danmcp commented Jun 30, 2024

Followup to #1369

This PR is using the new serving backends and support the vllm and llama_cpp path. It replaces temporary code serving vllm directly.

Checklist:

  • [ x] Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • [x ] Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@mergify mergify bot added the ci-failure PR has at least one CI failure label Jun 30, 2024
@mergify mergify bot added testing Relates to testing ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jun 30, 2024
@mergify mergify bot removed the ci-failure PR has at least one CI failure label Jun 30, 2024
@danmcp danmcp changed the title Use new vllm backend Use new vllm backend for evaluate Jun 30, 2024
@danmcp danmcp changed the title Use new vllm backend for evaluate Use new vllm/llama-cpp backends for evaluate Jun 30, 2024
@danmcp danmcp requested review from alimaredia and cdoern June 30, 2024 15:58
@nathan-weinberg nathan-weinberg self-requested a review July 1, 2024 01:09
nathan-weinberg
nathan-weinberg previously approved these changes Jul 1, 2024
Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like @alimaredia and @cdoern to signoff but apart from one logging comment LGTM

src/instructlab/model/evaluate.py Show resolved Hide resolved
@mergify mergify bot added the one-approval PR has one approval from a maintainer label Jul 1, 2024
src/instructlab/configuration.py Outdated Show resolved Hide resolved
src/instructlab/model/evaluate.py Outdated Show resolved Hide resolved
src/instructlab/model/evaluate.py Outdated Show resolved Hide resolved
src/instructlab/model/evaluate.py Outdated Show resolved Hide resolved
src/instructlab/model/evaluate.py Outdated Show resolved Hide resolved
@mergify mergify bot added ci-failure PR has at least one CI failure and removed one-approval PR has one approval from a maintainer labels Jul 1, 2024
@mergify mergify bot removed the ci-failure PR has at least one CI failure label Jul 1, 2024
@danmcp danmcp force-pushed the evalandvllm branch 2 times, most recently from 0167d22 to d9306a2 Compare July 1, 2024 14:49
@danmcp
Copy link
Contributor Author

danmcp commented Jul 1, 2024

Will need to adjust to changes in #1531

src/instructlab/configuration.py Outdated Show resolved Hide resolved
src/instructlab/model/evaluate.py Show resolved Hide resolved
src/instructlab/model/evaluate.py Show resolved Hide resolved
src/instructlab/model/evaluate.py Show resolved Hide resolved
src/instructlab/model/evaluate.py Show resolved Hide resolved
@mergify mergify bot added the ci-failure PR has at least one CI failure label Jul 1, 2024
@danmcp
Copy link
Contributor Author

danmcp commented Jul 1, 2024

Looks like pylint is catching a dep issue outside this code:

Error: src/instructlab/model/train.py:11:0: E0611: No name 'run_training' in module 'instructlab.training' (no-name-in-module)

@alimaredia alimaredia self-requested a review July 2, 2024 01:15
alimaredia
alimaredia previously approved these changes Jul 2, 2024
@mergify mergify bot added the one-approval PR has one approval from a maintainer label Jul 2, 2024
@danmcp danmcp mentioned this pull request Jul 2, 2024
3 tasks
cdoern
cdoern previously approved these changes Jul 2, 2024
Copy link
Contributor

@cdoern cdoern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right structure I think in terms of config. I might do a follow up to make the option overriding work as it does in the other cmds.

The way I set it up for train and @alimaredia did for serve is that in configuration.py the default_map in the context is set to have the right key->value pairs that will correspond to flag names in the specific cmd.

This is a clean solution though too and gets it all working, so it gets my +1 !

@mergify mergify bot removed the one-approval PR has one approval from a maintainer label Jul 2, 2024
Copy link
Contributor

@leseb leseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I think it's worth a note in the CHANGELOG.md. Thanks!

@danmcp danmcp dismissed stale reviews from cdoern and alimaredia via 1f4c177 July 2, 2024 12:55
@mergify mergify bot added the ci-failure PR has at least one CI failure label Jul 2, 2024
@mergify mergify bot added the CI/CD Affects CI/CD configuration label Jul 2, 2024
Signed-off-by: Dan McPherson <dmcphers@redhat.com>
@mergify mergify bot removed the ci-failure PR has at least one CI failure label Jul 2, 2024
@mergify mergify bot added one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jul 2, 2024
@cdoern
Copy link
Contributor

cdoern commented Jul 2, 2024

macos has been flaky recently, rerunning

@mergify mergify bot removed one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jul 2, 2024
@mergify mergify bot merged commit 08a9476 into instructlab:main Jul 2, 2024
@ktam3 ktam3 added this to the 0.18.0 milestone Jul 15, 2024
@danmcp danmcp deleted the evalandvllm branch August 7, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD Affects CI/CD configuration testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.