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

Model classification api #7742

Merged
merged 20 commits into from
Mar 18, 2025
Merged

Model classification api #7742

merged 20 commits into from
Mar 18, 2025

Conversation

jazzhaiku
Copy link
Collaborator

@jazzhaiku jazzhaiku commented Mar 6, 2025

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:

  • Is backwards compatible with the existing probe.
  • Encapsulates fields and logic in a single class, keeping things self-contained and easy to modify safely.

Below is a minimal toy example illustrating the proposed new structure:

class MinimalConfigExample(ModelConfigBase):
    type: ModelType = ModelType.Main
    format: ModelFormat = ModelFormat.Checkpoint
    fun_quote: str

    @classmethod
    def matches(cls, mod: ModelOnDisk) -> bool:
        return mod.path.suffix == ".json"

    @classmethod
    def parse(cls, mod: ModelOnDisk) -> dict[str, Any]:
        with open(mod.path, "r") as f:
            contents = json.load(f)

        return {
            "fun_quote": contents["quote"],
            "base": BaseModelType.Any,
        }

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

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

@github-actions github-actions bot added python PRs that change python files Root backend PRs that change backend files services PRs that change app services frontend PRs that change frontend files python-tests PRs that change python tests python-deps PRs that change python dependencies labels Mar 6, 2025
@jazzhaiku
Copy link
Collaborator Author

Having a review of my own code here: for simplicity, the legacy_probe decorator and concrete_subclasses() function could be replaced with 2 sets: UsingLegacyProbe and UsingModelClassify.

@jazzhaiku jazzhaiku force-pushed the model-classification-api branch from bf45a3c to 907d960 Compare March 10, 2025 21:38
Copy link
Contributor

@RyanJDick RyanJDick left a 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.

@psychedelicious psychedelicious self-requested a review March 13, 2025 03:12
Copy link
Collaborator

@psychedelicious psychedelicious left a 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.

@psychedelicious psychedelicious merged commit 133a7fd into main Mar 18, 2025
15 checks passed
@psychedelicious psychedelicious deleted the model-classification-api branch March 18, 2025 04:25
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 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants