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

Enable sane naming of registered objects with defaults #429

Merged
merged 7 commits into from
Nov 12, 2024
Merged

Conversation

ashwinb
Copy link
Contributor

@ashwinb ashwinb commented Nov 12, 2024

What does this PR do?

This is a follow-up to #425. That PR allows for specifying models in the registry, but each entry needs to look like:

- identifier: ...
  provider_id: ...
  provider_resource_identifier: ...

This is headache-inducing.

The current PR makes this situation better by adopting the shape of our APIs. Namely, we need the user to only specify model-id. The rest should be optional and figured out by the Stack. You can always override it.

Here's what example ollama "full stack" registry looks like (we still need to kill or simplify shield_type crap):

models:
- model_id: Llama3.2-3B-Instruct
- model_id: Llama-Guard-3-1B
shields:
- shield_id: llama_guard
  shield_type: llama_guard

Test Plan

See test plan for #425. Re-ran it.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 12, 2024
@ashwinb ashwinb changed the base branch from main to models_in_registry November 12, 2024 18:24
@@ -9,7 +9,7 @@
import pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow up: rebase and update eval_task/datasets/scoring_functions fixtures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed them now

@json_schema_type
class VectorMemoryBankParams(BaseModel):
class VectorMemoryBank(MemoryBankResourceMixin):
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - I don't think the actual types should change. But I will regenerate anyway @yanxi0830

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. There are a few small changes. I am cleaning up shields anyway in the next PR so there will be more changes there.



@json_schema_type
class ModelInput(CommonModelFields):
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if the register_model should also use ModelInput?

async def register_model(
        self,
        model_id: str,
        provider_model_id: Optional[str] = None,
        provider_id: Optional[str] = None,
        metadata: Optional[Dict[str, Any]] = None,
    ) -> Model: ...

If seems like we have 3 different structures now associated with a resource?

  • Model --> datastrcuture for model fields
  • ModelInput --> for specifying StackRunConfig pre-registration
  • register_model --> flattened args for ModelInput?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct. For now, I left it as now because I think register_model()'s API looks nice as is. Adding a structure to it makes it feel unnecessarily annoying. And internally, ModelInput does get destructured and passed to register_model() so any incompatibility is immediately flagged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also ModelInput is purely for the Stack-server-side developer. Only necessary and used in StackRunConfig. Maybe I should think about moving it there, but it helps to keep it here to make it consistent perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will kill @json_schema_type from these input fields for sure. They should not be in OpenAPI schema.

Base automatically changed from models_in_registry to main November 12, 2024 18:58
@ashwinb ashwinb merged commit 09269e2 into main Nov 12, 2024
1 of 2 checks passed
@ashwinb ashwinb deleted the easier_naming branch November 12, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants