Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

Implements --gpus option for eval - #1553

Merged
mergify[bot] merged 1 commit into
instructlab:mainfrom
danmcp:evalgpus
Jul 8, 2024
Merged

Implements --gpus option for eval#1553
mergify[bot] merged 1 commit into
instructlab:mainfrom
danmcp:evalgpus

Conversation

@danmcp

@danmcp danmcp commented Jul 2, 2024

Copy link
Copy Markdown
Contributor

Implements --gpus option for evaluate.

Issue resolved by this Pull Request:
Resolves #1549

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 testing Relates to testing label Jul 2, 2024
@danmcp
danmcp marked this pull request as draft July 2, 2024 01:34
@danmcp
danmcp marked this pull request as ready for review July 2, 2024 15:56
@danmcp
danmcp requested a review from cdoern July 2, 2024 15:57
Comment thread src/instructlab/model/evaluate.py Outdated
nathan-weinberg
nathan-weinberg previously approved these changes Jul 2, 2024

@nathan-weinberg nathan-weinberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pending the discussion this LGTM

@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 2, 2024
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jul 2, 2024
nathan-weinberg
nathan-weinberg 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

@russellb russellb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

one other comment about the config file addition

Comment thread src/instructlab/configuration.py Outdated
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jul 3, 2024
nathan-weinberg
nathan-weinberg previously approved these changes Jul 3, 2024
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 3, 2024
Comment thread src/instructlab/model/evaluate.py
Comment thread src/instructlab/model/evaluate.py
@russellb
russellb dismissed their stale review July 4, 2024 03:47

my requested change was incorporated

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please update the CHANGELOG.md. Thanks!

@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 4, 2024
@danmcp

danmcp commented Jul 4, 2024

Copy link
Copy Markdown
Contributor Author

Please update the CHANGELOG.md. Thanks!

Done!

@mergify mergify Bot added CI/CD Affects CI/CD configuration and removed ci-failure PR has at least one CI failure labels Jul 4, 2024
leseb
leseb previously approved these changes Jul 5, 2024

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a thought for later: I hate to see users shooting themselves in the foot, even if the flags empower them to do so. I'm wondering if we should log a warning when someone provides a ridiculously high value for --gpus. This consideration applies to other flags as well.

@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 5, 2024
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jul 5, 2024
alimaredia
alimaredia previously approved these changes Jul 5, 2024
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 5, 2024
@mergify

mergify Bot commented Jul 6, 2024

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @danmcp please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase This Pull Request needs to be rebased label Jul 6, 2024
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed needs-rebase This Pull Request needs to be rebased one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jul 6, 2024
Comment thread src/instructlab/model/evaluate.py Outdated
Comment thread src/instructlab/model/evaluate.py
Comment thread CHANGELOG.md
Comment thread src/instructlab/model/evaluate.py
Comment thread src/instructlab/model/evaluate.py
Signed-off-by: Dan McPherson <dmcphers@redhat.com>
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 8, 2024
@alimaredia
alimaredia removed request for cdoern and russellb July 8, 2024 15:07
@mergify
mergify Bot merged commit 72d2595 into instructlab:main Jul 8, 2024
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jul 8, 2024
@danmcp
danmcp deleted the evalgpus branch August 7, 2024 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ilab model evaluate should take #gpus to send to vllm/llama-cpp

6 participants