Support search traces by prompt version#18906
Conversation
- Removed the deprecated LINKED_PROMPTS_TAG_KEY from prompt constants. - Updated all references to linked prompts in the codebase to use TraceTagKey.LINKED_PROMPTS. - Added validation for prompts filter in search functionality to ensure only exact matches are allowed. - Enhanced error handling for invalid prompts filter format. This change centralizes the management of linked prompts and improves the clarity of the codebase. Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
|
Documentation preview for 9d623ce is available at: Changed Pages (1)
More info
|
Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
There was a problem hiding this comment.
Can we add some documentation under /genai/prompt-registry/ as well? Maybe it's worth having a single page explains link between prompt and traces. Maybe follow-up after adding UI feature to jump to a list of traces from the prompt page.
There was a problem hiding this comment.
Yeah, let me add the doc section after adding UI changes.
|
|
||
| if SearchTraceUtils.is_tag(key_type, comparator): | ||
| entity = SqlTraceTag | ||
| # Special handling for prompts filter: only support exact match with name/version |
There was a problem hiding this comment.
Can we actually use the entity association somehow? Since traces are stored in the experiment, search query goes through tracking store anyway.
There was a problem hiding this comment.
We could migrate the linkage to EntityAssociation, but what the motivation would be? For storing more structured linkage?
There was a problem hiding this comment.
For performance. Entity association table stores one row per prompt+trace combination, ooposed to a tag that combines multiple prompts into one row, so we can use the exact match condition and benefit from the index of destination ID (WHERE destination_type = 'prompt' and destination_id = '<name/version>'). This should be much more efficient than LIKE on tag values.
There was a problem hiding this comment.
Understood, do we think the efficiency gain is worth breaking the backward compatibility right now? Current search does not require any data model change, and we can always switch to entity association if users report search slowness.
There was a problem hiding this comment.
Understood, do we think the efficiency gain is worth breaking the backward compatibility right now?
Yes. Afaik we don't provide any place users can see the link between tag and prompt in OSS side, then I don't think it breaks anything. The migration becomes harder when we do it after adding the search functionality.
If you mean Databricks side, we can keep tag logging for DBX. They also plan to migrate to entity association table fwiw.
There was a problem hiding this comment.
I think consolidating the linkage in entity association table and populate trace fields (either tag or metadata) from the entity association is clearer since we can keep single source of truth.
Yes, it removes duplication, but one downside is every prompt load needs to make additional SQL query, even if the trace is not linked to any prompt. Since tags are stored in TraceInfo, searching traces also triggers that for every prompt.
Also unfortunately Databricks UI already shows prompts link based on trace tag (which is different from OSS). This means that, even after dbx migrate to entity table, we cannot remove the tag dependence at least for a certain period😔
This means that, the end state will be
Option 1 (yours):
- Write: Set the tag in dbx only. Store a record in entity association table.
- Read: Populate tag/metadata at load time.
Option 2:
- Write: Set the tag and store to entity association table always.
- Read: none
I'm not sure if the state 1 is cleaner than 2. The single source of truth is nice in general, but not always the best - for example, we can think it as a kind of databse denormalization - we denormalize the info to entity table for search optimization.
There was a problem hiding this comment.
I got your point, but do you think the single additional query for retrieving associations is that problematic? As you said earlier, it should be quick since it relies on database indices, and as I implemented, it does not cause N+1 issue by using batch loading. Btw not necessarily objecting to option 2, we can go with dual writing if skipping one query is really worth database denormalization.
There was a problem hiding this comment.
Btw, there are several todo comments to remove tag based linkage, didn't we want to remove the tag based approach?
mlflow/mlflow/tracing/utils/prompt.py
Line 8 in b3f0e30
There was a problem hiding this comment.
I'm fine with populating it to tags if you feel strongly about it. For the TODO comment, I didn't expect to have "linked prompt" tab in the trace UI but just to have a button like "see linked prompts" that jumps to search page:)
There was a problem hiding this comment.
I see, shall we defer the decision by implementing option 2 for now? Option 2 -> option 1 is feasible while the opposite is not.
…y across documentation and codebase - Changed all instances of 'prompts' to 'prompt' in the search_traces API documentation and related code. - Updated error messages and test cases to reflect the new filter name. - Ensured that the filter only supports exact matches with the format "name/version". This change enhances clarity and consistency in the API usage. Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
…low into feat/prompt/search-traces
- Updated the filter validation logic to check for exactly one '/' in the prompt value, improving error handling for invalid inputs. - Adjusted the test cases to include a new scenario for invalid prompt format. This change enhances the robustness of the prompts filter functionality. Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
|
/review Only review ✅ Review completed. Review OutputPerfect! I've successfully reviewed the PR and added a comment suggesting a cleaner approach using pattern matching. SummaryI reviewed PR #18906 focusing on Issue Found: The code uses the verbose approach of checking Suggestion: Replace this with pattern matching ( The review comment has been posted to the PR with a code suggestion that maintainers can apply with one click: #18906 (comment) |
- Introduced the `linkPromptsToTrace` RPC to associate multiple prompt versions with a trace, enhancing prompt tracking workflows. - Implemented the necessary data structures and request handlers to support this functionality. - Updated the tracking store interfaces to allow linking prompts to traces using entity associations. - Added tests to verify the correct behavior of the new API and its integration with the existing system. This change improves the ability to manage prompt versions in relation to traces, facilitating better tracking and organization of prompt usage. Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
…Store - Added a method to populate the LINKED_PROMPTS tag from entity associations for traces that lack this tag, ensuring backward compatibility. - Updated the get_trace_info method to call the new population method. - Enhanced search_traces to include the LINKED_PROMPTS tag for relevant traces. - Added unit tests to verify the correct population of LINKED_PROMPTS in various scenarios. This change improves trace management by maintaining compatibility with existing data structures. Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
…traces Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
0d368f2 to
643a2a9
Compare
Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
Signed-off-by: Tomu Hirata <tomu.hirata@gmail.com>
🛠 DevTools 🛠
Install mlflow from this PR
For Databricks, use the following command:
Related Issues/PRs
n/a
What changes are proposed in this pull request?
This PR introduces a new
promptsfilter forsearch_traces. As part is this change, we migrate the linkage storage from trace tag to entity association table of tracking store.How is this PR tested?
Does this PR require documentation update?
Release Notes
Is this a user-facing change?
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/tracking: Tracking Service, tracking client APIs, autologgingarea/models: MLmodel format, model serialization/deserialization, flavorsarea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/evaluation: MLflow model evaluation features, evaluation metrics, and evaluation workflowsarea/gateway: MLflow AI Gateway client APIs, server, and third-party integrationsarea/prompts: MLflow prompt engineering features, prompt templates, and prompt managementarea/tracing: MLflow Tracing features, tracing APIs, and LLM tracing functionalityarea/projects: MLproject format, project running backendsarea/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesHow should the PR be classified in the release notes? Choose one:
rn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notesShould this PR be included in the next patch release?
Yesshould be selected for bug fixes, documentation updates, and other small changes.Noshould be selected for new features and larger changes. If you're unsure about the release classification of this PR, leave this unchecked to let the maintainers decide.What is a minor/patch release?
Bug fixes, doc updates and new features usually go into minor releases.
Bug fixes and doc updates usually go into patch releases.