-
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
New API for model classification #7728
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e6099f7
to
78b8451
Compare
I just looked into why the re-formatting was needed. It seems like ruff's format rules changed somewhere between We should pin the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
backend
PRs that change backend files
frontend
PRs that change frontend files
invocations
PRs that change invocations
python
PRs that change python files
python-deps
PRs that change python dependencies
python-tests
PRs that change python tests
Root
services
PRs that change app services
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:1. Define fields 'type' and 'format' as class attributes.
2. Override methods 'matches' and 'parse':
matches
performs a quick check to determine if the config matches the model.This doesn't need to be a perfect test - the aim is to eliminate unlikely matches quickly before parsing.
parse
returns a dictionary with any additional fields needed to construct the model.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.
Note: some unrelated linting changes included to make
ruff
happy.Related Issues / Discussions
QA Instructions
Merge Plan
Checklist
What's New
copy (if doing a release after this PR)