-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Model classification api #7742
Model classification api #7742
Conversation
Having a review of my own code here: for simplicity, the |
bf45a3c
to
907d960
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor comment, otherwise looks good to me. I ran a few manual smoke tests; no issues.
…eAI into model-classification-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Let's wait until v5.8.0 is tagged and released to merge.
Summary
The goal of this PR is to make it easier to add an new config type.
This scope of this PR is to integrate the API and does not include adding new configs (outside tests) or porting existing ones.
One of the glaring issues of the existing legacy probe is that the logic for each type is spread across multiple classes and intertwined with the other configs. This means that adding a new config type (or modifying an existing one) is complex and error prone.
This PR attempts to remedy this by providing a new API for adding configs that:
Below is a minimal toy example illustrating the proposed new structure:
To create a new config type, one needs to inherit from
ModelConfigBase
and implement its interface.The code falls back to the legacy model probe for existing models using the old API.
This allows us to incrementally port the configs one by one.
Related Issues / Discussions
QA Instructions
Merge Plan
Checklist
What's New
copy (if doing a release after this PR)