-
Notifications
You must be signed in to change notification settings - Fork 104
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
Refactor Assistant builder: use Vaults, use Context #6768
Refactor Assistant builder: use Vaults, use Context #6768
Conversation
fb3d3f0
to
88abec2
Compare
48fa4be
to
3c3285a
Compare
front/components/assistant_builder/AssistantBuilderDataSourceModal.tsx
Outdated
Show resolved
Hide resolved
front/components/assistant_builder/AssistantBuilderDataSourceModal.tsx
Outdated
Show resolved
Hide resolved
@flvndvd feedback adressed, feel free to re-review. |
15eaf10
to
c6a9600
Compare
7d4149c
to
88f9ca6
Compare
@@ -357,7 +357,7 @@ function DataSourcesSection({ | |||
<div className="flex flex-col gap-1"> | |||
<ManagedDataSourceDocumentModal |
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.
Note to self, move to DataSourceViewDocumentModal
once this PR is merged.
front/components/assistant_builder/AssistantBuilderDataSourceModal.tsx
Outdated
Show resolved
Hide resolved
@@ -62,7 +62,7 @@ const RetrievalActionConfigurationSchema = t.type({ | |||
dataSources: t.array( | |||
t.type({ | |||
dataSourceId: t.string, | |||
dataSourceViewId: t.union([t.string, t.null]), | |||
dataSourceViewId: t.string, |
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 will break the assistant builder for users already on the builder when we release.
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 do you propose ?
581a3d1
to
cecd6d8
Compare
cecd6d8
to
ba1c0ff
Compare
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 left a few minor comments, but they aren't critical. Let's go ahead and merge this PR since it is holding up several others. We can address those comments in a subsequent PR.
@@ -446,7 +444,7 @@ function DataSourceSelectedNodes({ | |||
|
|||
return ( | |||
<> | |||
{dataSourceSelectedNodes.nodes.map((node: ContentNode) => ( | |||
{dataSourceSelectedNodes.nodes.map((node: LightContentNode) => ( |
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.
Given that we have not yet changed useDataSourceContentNodes
this should still be a ContentNode
@@ -322,7 +322,7 @@ export default function AssistantBuilder({ | |||
}); | |||
} else { | |||
await mutate( | |||
`/api/w/${owner.sId}/data_sources/${slackDataSource?.name}/managed/slack/channels_linked_with_agent` | |||
`/api/w/${owner.sId}/data_sources/${slackDataSource?.dataSource.name}/managed/slack/channels_linked_with_agent` |
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.
nit
but slackDataSource
should be named slackDataSourceView
here.
import { CONNECTOR_CONFIGURATIONS } from "@app/lib/connector_providers"; | ||
import { AssistantBuilderContext } from "@app/components/assistant_builder/AssistantBuilderContext"; | ||
import { orderDatasourceViewByImportance } from "@app/lib/assistant"; | ||
import { getConnectorProviderLogoWithFallback } from "@app/lib/connector_providers"; | ||
import { getDisplayNameForDataSource } from "@app/lib/data_sources"; | ||
|
||
export default function PickDataSource({ |
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.
Follow up item, renamed this component to PickDataSourceView
.
@@ -143,7 +143,9 @@ export function EditVaultManagedDataSourcesViews({ | |||
}} | |||
owner={owner} | |||
systemVaultDataSourceViews={systemVaultDataSourceViews.filter( | |||
(ds) => ds.connectorProvider && ds.connectorProvider !== "webcrawler" | |||
(dsv) => |
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.
Follow up item we can use isManaged
here.
const defaultVaults = [await VaultResource.fetchWorkspaceGlobalVault(auth)]; | ||
const dataSourceViews = await DataSourceViewResource.listByVaults( | ||
auth, | ||
defaultVaults | ||
); |
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.
nit
const defaultVaults = [await VaultResource.fetchWorkspaceGlobalVault(auth)]; | |
const dataSourceViews = await DataSourceViewResource.listByVaults( | |
auth, | |
defaultVaults | |
); | |
const globalVault = await VaultResource.fetchWorkspaceGlobalVault(auth); | |
const dataSourceViews = await DataSourceViewResource.listByVaults( | |
auth, | |
[globalVault] | |
); |
// Used when returning an agent with status 'disabled_by_admin' | ||
const dummyModelConfiguration = { | ||
providerId: GPT_4O_MODEL_CONFIG.providerId, | ||
modelId: GPT_4O_MODEL_CONFIG.modelId, | ||
temperature: 0, | ||
}; | ||
|
||
type PrefetchedDataSourcesType = { |
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.
Follow up item, rename.
@@ -13,6 +13,8 @@ import type { | |||
DataSourceViewType, | |||
WhitelistableFeature, | |||
} from "@dust-tt/types"; | |||
import type { LucideIcon } from "lucide-react"; |
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.
Wait we should not use any of those icons 🤔
} | ||
await makePatchRequest( | ||
transcriptConfigurationId, | ||
{ | ||
dataSourceId: dataSource ? dataSource.name : null, | ||
dataSourceId: dataSource ? dataSource.dataSource.name : null, |
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.
Follow up item rename to dataSourceView
.
* Refactor Assistant builder: use Vaults, use Context * Fix: wrong key for actions initial configurations * Fix: force close when picking a table from a folder
Description
This PR is about using Vaults (& DataSourceViews) in the assistant builder flow.
The main changes to make it work with
DataSourceView
:AssistantBuilderDataSourceModal
store configurations mapping by dataSourceView.sId instead of dataSource.sIdLightContentNode
(returned by Vault's api) instead ofContentNode
(old api).useVaultDataSourceViewContent
instead ofuseConnectorPermissions
.Note: it will break using "@dust" locally.
Risk
Break assistant builder
Deploy Plan
Deploy
front-edge
, test, deployfront