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

New API for model classification #7728

Closed
wants to merge 11 commits into from
Closed

New API for model classification #7728

wants to merge 11 commits into from

Conversation

jazzhaiku
Copy link
Collaborator

@jazzhaiku jazzhaiku commented Mar 4, 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:

1. Define fields 'type' and 'format' as class attributes.
2. Override methods 'matches' and 'parse':

  • The method 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.
  • The method 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

  • 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 python-tests PRs that change python tests python-deps PRs that change python dependencies invocations PRs that change invocations labels Mar 4, 2025
@jazzhaiku jazzhaiku changed the title Model probe refactor New API for model classification Mar 4, 2025
@jazzhaiku jazzhaiku force-pushed the model-probe-refactor branch from e6099f7 to 78b8451 Compare March 4, 2025 08:15
@github-actions github-actions bot added the frontend PRs that change frontend files label Mar 4, 2025
@RyanJDick
Copy link
Collaborator

I just looked into why the re-formatting was needed. It seems like ruff's format rules changed somewhere between 0.6.2 (the version I was running locally) and 0.9.9 (the current version). I didn't bother to narrow it down further than that.

We should pin the ruff version: "ruff~=0.9.9", "ruff-lsp~=0.0.62". Do you mind splitting out the reformatting into a separate PR to keep the diff on this one clean?

@jazzhaiku jazzhaiku closed this Mar 5, 2025
@jazzhaiku jazzhaiku reopened this Mar 5, 2025
@jazzhaiku jazzhaiku closed this Mar 6, 2025
@jazzhaiku jazzhaiku reopened this Mar 6, 2025
@jazzhaiku jazzhaiku closed this Mar 6, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants