Skip to content

Auto collect dynamic SUT factories#1501

Merged
bkorycki merged 18 commits into
mainfrom
auto-collect-factories
Mar 19, 2026
Merged

Auto collect dynamic SUT factories#1501
bkorycki merged 18 commits into
mainfrom
auto-collect-factories

Conversation

@bkorycki
Copy link
Copy Markdown
Contributor

No description provided.

@bkorycki bkorycki requested review from superdosh and wpietri March 12, 2026 19:23
@bkorycki bkorycki requested a review from a team as a code owner March 12, 2026 19:23
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 12, 2026

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

@bkorycki bkorycki temporarily deployed to Scheduled Testing March 12, 2026 19:27 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing March 12, 2026 19:31 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing March 12, 2026 19:31 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing March 12, 2026 23:59 — with GitHub Actions Inactive
Copy link
Copy Markdown
Contributor

@superdosh superdosh left a comment

Choose a reason for hiding this comment

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

This looks good to me overall, though I don't love the name.

Comment thread src/modelgauge/suts/anthropic_sut_factory.py
Comment thread src/modelgauge/dynamic_sut_factory.py Outdated
Comment thread tests/modelgauge_tests/test_dynamic_sut_factory.py
Copy link
Copy Markdown
Contributor

@wpietri wpietri left a comment

Choose a reason for hiding this comment

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

I like the theory, but

  1. How do we know it works? I'd like to see a test that at least N factories are found under normal run conditions, and
  2. How do we make sure it keeps working? That is, if somebody adds a new factory, are we sure they'll do the right thing to make sure it shows up?

Comment thread src/modelgauge/sut_factory.py Outdated
@bkorycki bkorycki temporarily deployed to Scheduled Testing March 16, 2026 16:34 — with GitHub Actions Inactive
@bkorycki bkorycki temporarily deployed to Scheduled Testing March 16, 2026 16:55 — with GitHub Actions Inactive
@bkorycki
Copy link
Copy Markdown
Contributor Author

  1. A unit test for the actual collection is a great idea. I just added that.
  2. All it takes for a new factory to show up is to have it inherit from DynamicDriverSUTFactory. And I would expect that every time someone adds a new dynamic factory that they test that it works with some CLI command as a part of their normal workflow. We could also add a unit test for every dynamic factory to ensure it gets loaded. Though that requires people to remember to add that same unit test every time they add a new dynamic factory, so I don't know how much that actually accomplishes.

@bkorycki bkorycki requested a review from wpietri March 16, 2026 17:00
@bkorycki bkorycki temporarily deployed to Scheduled Testing March 16, 2026 18:38 — with GitHub Actions Inactive
* remove deprecated models

* nvidia nim sut factory
@bkorycki bkorycki temporarily deployed to Scheduled Testing March 19, 2026 16:24 — with GitHub Actions Inactive
@bkorycki bkorycki merged commit 46c31fe into main Mar 19, 2026
2 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 19, 2026
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.

3 participants