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

Modelbench SUT cleanup + testing infra improvements #754

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

bkorycki
Copy link
Contributor

These are some initial improvements along the road to removing the SUT wrapper class in modelbench. I'm splitting the changes up into smaller PRs to make the more manageable to review.

Changes in this PR:

  • Get rid of DEFAULT_SUTS list, which was leftover from v0.5. The SUTs in that list aren't really that relevant, and I think it's reasonable to assume + expect that user will specify SUT uids at the command line.
  • Relatedly, you now must set at least 1 --sut <uid> arg when running the CLI.
  • Modelbench does not register SUTs. I think we should keep SUT registration in modelgauge in order to avoid registering an identical SUT to multiple UIDs. Also, none of those SUTs that modelbench registered were used in v1.0 anyway.
  • Testing infra improvements. I created a conftest file to register a fake SUT during setup. This removes a lot of code duplication in the tests (i.e. creating + registering dummy SUTs). And the centralization will make it easier to fully murder the ModelGaugeSut wrapper.

@bkorycki bkorycki requested a review from a team as a code owner December 16, 2024 22:36
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 16, 2024 22:36 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 16, 2024 22:36 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 16, 2024 22:36 — with GitHub Actions Inactive
Copy link

github-actions bot commented Dec 16, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

Copy link
Contributor

@rogthefrog rogthefrog left a comment

Choose a reason for hiding this comment

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

Having an easy-to-read sut ID list would be super nice.

src/modelbench/run.py Outdated Show resolved Hide resolved
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 16, 2024 23:43 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 16, 2024 23:43 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 16, 2024 23:43 — with GitHub Actions Inactive
@bkorycki bkorycki mentioned this pull request Dec 17, 2024
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 18, 2024 20:48 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 18, 2024 20:48 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing December 18, 2024 20:48 — with GitHub Actions Inactive
@bkorycki bkorycki merged commit b9c0b7e into remove-0.5-code Dec 18, 2024
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants