-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Add MLflow Prompt Registry provider #4170
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
base: main
Are you sure you want to change the base?
feat: Add MLflow Prompt Registry provider #4170
Conversation
mattf
left a comment
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.
@williamcaban this is the right direction
few things jump out for me -
- the existing impl needs to be moved to be an inline::reference impl (let the llm know it's in src/llama_stack/core/prompts/prompts.py)
- mlflow credential handling needs to be added, should follow the pattern in inference (provider-data backstopped by config)
- tests don't need to be standalone
dd0758c to
cb69e41
Compare
cb69e41 to
1e68fa8
Compare
|
PR is now ready for review and includes the following updates:
|
| """Create ID mapper instance.""" | ||
| return PromptIDMapper(use_metadata=True) | ||
|
|
||
| def test_to_mlflow_name_valid_id(self, mapper): |
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.
some of these tests could be shortened. i understand Claude often generates them easily but sometimes they're a little excessive.
franciscojavierarceo
left a comment
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.
some small nits but this is looking very exciting!
| @@ -0,0 +1,92 @@ | |||
| --- | |||
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.
thank you for adding the prompts docs!!
we also have this in the UI, maybe we should mention it?
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.
Are you referring to making it available in the UI? If so, could that be a follow-up PR? I would prefer to avoid adding more to this one.
mattf
left a comment
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.
why do we need to maintain a mapping from prompt id to mlflow prompt name?
The idea is to distinguish Llama Stack-managed prompts from other prompts that might exist in the same MLflow registry. |
041cd2e to
8f150ec
Compare
when would a deployer want to set use_metadata_tags=False? can we always use metadata and skip the id/name translations? |
We can remove that option because, in practice, setting it to false has significant downsides. Let me remove the option. |
thanks. do we still need to have the id mapping? |
mattf
left a comment
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.
@williamcaban why create a func for token extraction and a client but then not use them?
by requesting pr review you're asking others to read all this code (over 3k lines). please review it all before requesting.
| if self.config.auth_credential is not None: | ||
| import os | ||
|
|
||
| # MLflow reads MLFLOW_TRACKING_TOKEN from environment | ||
| os.environ["MLFLOW_TRACKING_TOKEN"] = self.config.auth_credential.get_secret_value() | ||
| logger.debug("Set MLFLOW_TRACKING_TOKEN from config auth_credential") |
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.
because you use env.MLFLOW_TRACKING_TOKEN in the sample_run_config, the auth_credential will already be set from the MLFLOW_TRACKING_TOKEN.
is setting MLFLOW_TRACKING_TOKEN the only way to communicate the token to the client?
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.
This can be either—an environment variable at runtime or a setting in the config file.
| logger.debug("Set MLFLOW_TRACKING_TOKEN from config auth_credential") | ||
|
|
||
| # Initialize client | ||
| self.mlflow_client = MlflowClient() |
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.
this is unused.
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.
This is used in the edge case of managing prompts created outside Llama Stack. I'll update the code for clarity.
79a3454 to
7705651
Compare
Add a new remote provider that integrates MLflow's Prompt Registry with Llama Stack's prompts API, enabling centralized prompt management and versioning using MLflow as the backend. Features: - Full implementation of Llama Stack Prompts protocol - Support for prompt versioning and default version management - Automatic variable extraction from Jinja2-style templates - MLflow tag-based metadata for efficient prompt filtering - Flexible authentication (config, environment variables, per-request) - Bidirectional ID mapping (pmpt_<hex> ↔ llama_prompt_<hex>) - Comprehensive error handling and validation Implementation: - Remote provider: src/llama_stack/providers/remote/prompts/mlflow/ - Inline reference provider: src/llama_stack/providers/inline/prompts/reference/ - MLflow 3.4+ required for Prompt Registry API support - Deterministic ID mapping ensures consistency across conversions Testing: - 15 comprehensive unit tests (config validation, ID mapping) - 18 end-to-end integration tests (full CRUD workflows) - GitHub Actions workflow for automated CI testing with MLflow server - Integration test fixtures with automatic server setup Documentation: - Complete provider configuration reference - Setup and usage examples with code samples - Authentication options and security best practices Signed-off-by: William Caban <[email protected]> Co-Authored-By: Claude <[email protected]>
7705651 to
0e0d311
Compare
MLflow Prompt Registry Provider
Summary
This PR adds a new remote MLflow provider for the Prompts API, enabling centralized prompt management and versioning using MLflow's Prompt Registry (MLflow 3.4+).
What's New
Remote Provider:
remote::mlflowA production-ready provider that integrates Llama Stack's Prompts API with MLflow's centralized prompt registry, supporting:
{{ variable }}placeholdersQuick Start
1. Configure Llama Stack
Basic configuration with SQLite (default):
With PostgreSQL:
2. Use the Prompts API