-
Notifications
You must be signed in to change notification settings - Fork 21
Llm integration POC #1028
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?
Llm integration POC #1028
Conversation
…s allowed token count. Make conflicting libraries pydantic-ai and ag-ui optional; disabling agent route if not installed. Make search routes async and fix small bugs in query building.
CodSpeed Performance ReportMerging #1028 will not alter performanceComparing Summary
|
…hestrator-core into llm-integration
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1028 +/- ##
==========================================
- Coverage 85.14% 78.75% -6.40%
==========================================
Files 217 251 +34
Lines 10495 12387 +1892
Branches 1004 1214 +210
==========================================
+ Hits 8936 9755 +819
- Misses 1305 2372 +1067
- Partials 254 260 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dotenv run python main.py search semantic "Shop for an alligator store" | ||
... | ||
{ | ||
"path": "subscription.shop.shop_description", | ||
"value": "Kingswood reptiles shop" | ||
}, |
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.
We should make the examples more generic (also the ones below), since this is specific for the WFO instance where we built the initial POC.
...migrations/versions/schema/2025-08-12_52b37b5b2714_search_index_model_for_llm_integration.py
Outdated
Show resolved
Hide resolved
orchestrator/search/docs/running_local_text_embedding_inference.md
Outdated
Show resolved
Hide resolved
orchestrator/search/filters/base.py
Outdated
FilterCondition = ( | ||
DateFilter # DATETIME | ||
| NumericFilter # INT/FLOAT | ||
| StringFilter # STRING TODO: convert to hybrid search |
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.
do we need to make a ticket for this TODO?
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.
Im thinking that maybe this stringfilter should be removed altogether, its already possible to do a hybrid search by passing a user query, passing something like the top 5 results back to the agent will probably yield better results.
For things like booleans/product blocks , we already have the equality filter. Matching on exact text by letting the agent fill in a string will probably not work well.
…ndpoints for autocompleting paths and UI compatible operators per field type for frontend rendering.
… settings and instructions.
…ption records in response, improve highlighting
…hestrator-core into llm-integration
…d substring highlighting
3840906
to
e9597e1
Compare
…ith just a field name and value type. Support component contains/not contains filters.
except Exception as e: | ||
logger.warning(f"Failed to load schema for prompt: {e}") | ||
schema_info = " Schema temporarily unavailable" | ||
logger.error(f"Generated schema for agent prompt:\n{schema_info}") |
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.
I don't think this is suppose to be an error log?
# We are explicitly excluding 'traceback' and 'steps' | ||
# to avoid overloading the index with too much data. | ||
_process_fields_to_exclude: set[str] = { | ||
"traceback", |
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.
only excluding traceback
? update exclude list or the above comment
@@ -92,6 +92,34 @@ class AppSettings(BaseSettings): | |||
EXPOSE_SETTINGS: bool = False | |||
EXPOSE_OAUTH_SETTINGS: bool = False | |||
|
|||
# Pydantic-ai Agent settings | |||
AGENT_MODEL: str = "openai:gpt-4o-mini" # See pydantic-ai docs for supported models. |
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.
It might be nice to create a different settings class for LLM settings
def _extract_matching_field_from_filters(filters: FilterTree) -> MatchingField | None: | ||
"""Extract the first path filter to use as matching field for structured searches. | ||
|
||
TODO: Should we allow a list of matched fields in the MatchingField model? |
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.
what to do with this? new issue?
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.
Nice, that's a lot of work 🔥
Overall structure of the code is good, that's why I was able to leave a lot of questions and small remarks. I mean this as a good thing :)
@@ -18,7 +18,7 @@ jobs: | |||
options: --privileged | |||
services: | |||
postgres: | |||
image: postgres:15-alpine | |||
image: pgvector/pgvector:pg15 |
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.
What does this add on top of the normal postgres image? Is there documentation for the extra postgres configuration/extensions required?
search_params=search_params, | ||
db_session=db.session, | ||
pagination_params=pagination_params, | ||
) |
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.
Have there been any thoughts about implementing access control?
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 planned for later.
|
||
|
||
def build_agent_app() -> ASGIApp: | ||
if not app_settings.AGENT_MODEL or not app_settings.OPENAI_API_KEY: |
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.
These settings are strings that can't be None so by default it will be enabled. Since users need to configure the LLM setup, by default it should IMO be disabled with a bool variable like AGENT_ENABLED
|
||
return agent.to_ag_ui(deps=StateDeps(SearchState())) | ||
except Exception as e: | ||
logger.error("Agent init failed; serving disabled stub.", error=str(e)) |
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.
What kind of failures has this shown?
sa.Column("entity_id", postgresql.UUID, nullable=False), | ||
sa.Column("path", LtreeType, nullable=False), | ||
sa.Column("value", sa.Text, nullable=False), | ||
sa.Column("embedding", Vector(TARGET_DIM), nullable=True), |
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.
Does this require the database to have pgvector
installed?
entity_scores.join(entity_highlights, entity_scores.c.entity_id == entity_highlights.c.entity_id) | ||
) | ||
).cte("ranked_results") | ||
|
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.
Could we split this function up in one for the DB interaction part which produces an output, and another function that performs the below computations based on the former's output? And preferably also some unittests for the latter
@@ -0,0 +1,447 @@ | |||
from abc import ABC, abstractmethod |
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.
Maybe split up into a package with a module for each retriever type, it's a lot of scrolling now :)
|
||
def _quantize_score_for_pagination(self, score_value: float) -> BindParameter[Decimal]: | ||
"""Convert score value to properly quantized Decimal parameter for pagination.""" | ||
pas_dec = Decimal(str(score_value)).quantize(Decimal("0.000000000001")) |
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.
Should this change along with the SCORE_PRECISION if that ever changes?
If so maybe do something like f'{1 / 10**precision:.{precision}f}
|
||
if not matches: | ||
substring_pattern = re.escape(word) | ||
matches = list(re.finditer(substring_pattern, text, re.IGNORECASE)) |
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.
If a resulting text has both word and substring matches, wouldn't we want to highlight the substring matches as well?
|
||
class TypeDefinition(BaseModel): | ||
operators: list[FilterOp] | ||
valueSchema: dict[FilterOp, ValueSchema] |
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.
Is camelCase needed here?
pydantic-ai
andag-ui-protocol
need
pydantic >= 2.10
and>=2.11.2
respectively, this breaks some of the unit tests