Fetch data source view in agent configuration #6708
Conversation
flvndvd
changed the title
flvndvd
changed the title
| agentTablesConfigurationTables, | ||
| dustApps, | ||
| ] = await Promise.all([ | ||
| retrievalDatasourceConfigurationsPromise, | ||
| processDatasourceConfigurationsPromise, | ||
| fetchAgentRetrievalConfigurationsActions({ configurationIds, variant }), |
There was a problem hiding this comment.
Why is it ConfigurationActions ? Do you mean ActionConfigurations ?
| `Couldn't find topK for retrieval configuration ${retrievalConfig.id}} with 'custom' topK mode` | ||
| ); | ||
| } | ||
| const retrievalActions = |
There was a problem hiding this comment.
these are not actions, they are action configurations (naming is quite confusing IMO)
| import { DataSourceViewResource } from "@app/lib/resources/data_source_view_resource"; | ||
| import { DataSourceViewModel } from "@app/lib/resources/storage/models/data_source_view"; | ||
|
|
||
| export async function fetchAgentProcessConfigurationsActions({ |
There was a problem hiding this comment.
This will add latency. Before, we were getting the action config with the rest of action configs, and had a second step for all the "secondary" resources linked to action configs. Now we have an extra step
Seen IRL, I'll just go all the way in and fully migrate all action configurations to this pattern to maintain a consistent latency. Henry's concern is not about the additional database calls but rather the fact that these two functions are slightly slower compared to the others. |
| @@ -37,6 +37,12 @@ import { | |||
| DEFAULT_TABLES_QUERY_ACTION_NAME, | |||
| DEFAULT_WEBSEARCH_ACTION_NAME, | |||
| } from "@app/lib/api/assistant/actions/names"; | |||
| import { fetchBrowseActionsConfigurations } from "@app/lib/api/assistant/configuration/browse"; | |||
There was a problem hiding this comment.
nit: the double plural here feels a little weird to me, I would rather fetchBrowseActionConfigurations
| configurationIds: ModelId[]; | ||
| variant: "light" | "full"; | ||
| }): Promise<Map<ModelId, BrowseConfigurationType[]>> { | ||
| if (variant !== "full") { |
There was a problem hiding this comment.
TBH I feel it would be better to not pass variant to these functions, and just handle that in parent (i.e, not call the function at all if variant is not full)
But I can live with this
There was a problem hiding this comment.
Will give it some more love in a follow up PR.
Tested with all the dust apps locally (except table query). |
flvndvd
added
the
migration-ack
* Fetch data source view in agent configuration * Setup Sequelize relationship * Return both data source id and data source view id. * Refactor process action * Address naming comments from review * Refactor dust app fetching logic * Refactor table query actions fetch * Refactor websearch and browse fetch logic * ✨ * ✨
Description
This PR refactors the fetch agent configuration. Now, we fetch all data sources and any associated data source views for both retrieval and process action configurations. This change is necessary to pass these data source views to the core when performing any of these actions. Data source views are essential for conditioning access to a data source when reading documents from it. The code that calls core will be modified in a subsequent PR.
Additionally, I started splitting the fetch agent configuration code by action, with each file corresponding to a specific action’s fetch logic. Parallelization is still maintained. To keep the same "latency" we should move all the actions fetching logic to this pattern.
I left several TODOs since the UI is not yet aware of data source views. However, considering that the types are shared for both posting a configuration and fetching, this PR will break the builder UI for users currently on it. Given the current load, this should be manageable.
Risk
Safe to rollback.
Deploy Plan
No SQL migration, only some stuff to please Sequelize.