-
Notifications
You must be signed in to change notification settings - Fork 112
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
Assistant: Lib function createAgentConfiguration #1303
Conversation
8974e81
to
ad3f29a
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.
Left some comments 👍
front/lib/api/assistant/agent.ts
Outdated
name: name, | ||
pictureUrl: pictureUrl ?? null, | ||
scope: "workspace", | ||
workspaceId: owner.id, |
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 it allowNull:false?
front/lib/api/assistant/agent.ts
Outdated
} | ||
} | ||
|
||
return _getAgentConfigurationType({ |
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 not return what you've been passed?
9551c37
to
335ab51
Compare
Note: conflicts with main prolly needs rebasing as well :) |
4cec4b9
to
d2f9d8e
Compare
8157e78
to
72ba099
Compare
72ba099
to
b20d025
Compare
(arg as TimeFrame).duration !== undefined && | ||
(arg as TimeFrame).unit !== undefined | ||
); | ||
} | ||
|
||
// Retrieval specifies a list of data sources (with possible parent / tags filtering, possible "all" |
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.
you didn't move the comments?
auth: Authenticator, | ||
agentId: string, | ||
{ | ||
name, |
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 not generation related?
* We need to fetch the dataSources from the database from that. | ||
* We obvisously need to do as few queries as possible. | ||
*/ | ||
export async function _createAgentDataSourcesConfigData( |
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.
let's not export and call it createAgentDataSourceConfigurations
?
4b0e5cd
to
90a0e85
Compare
90a0e85
to
b49e22d
Compare
/** | ||
* Action > Retrieval | ||
*/ | ||
// Each AgentActionConfigurationType is capable of generating this type at runtime to specify which |
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 comment bleongs to AgentActionSpecification
?
/** | ||
* Retrieval Action config | ||
*/ | ||
export type RetrievalConfigurationType = { |
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 was moved up and away from the comment that explains it below.
This file diff should be minimal as really not much has changed yet there is 100s of diff here?
@@ -0,0 +1,352 @@ | |||
import { |
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.
Can we rename that file back to agent.ts for reviewing and then we can rename it in a subsequent PR. Makes it very hard to review otherwise?
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.
The retrieval configuration models should be in retrieval.ts also let's move them back there. This will also drastically reduce the diff and make it tractable to review 🙏
name: string; | ||
pictureUrl: string; | ||
status: AgentConfigurationStatus; | ||
generationConfigId: ModelId | 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.
This should be AgentGenerationConfigurationType and AgentActionConfigurationType from which you'll be able to get the ModelID
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.
You won't have to recreate them below. Simpler
}); | ||
|
||
// Return the config with Generation and Action if any | ||
const existingGeneration = agentConfig.generationConfigId |
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 not be here?
}) | ||
: null; | ||
|
||
const existingRetrivalConfig = agentConfig.retrievalConfigId |
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 not be here?
add5cbc
to
32afccc
Compare
I'd rather pick from this one to create smaller PRs! |
Closing and reopening smaller PRs! 👍🏻 |
This PR implements
createAgentConfiguration
.Review with hidden whitespaces!
A few other side-changes:
all
Action
level so removed fromAgentDataSourcesConfig