Skip to content
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

Closed
wants to merge 12 commits into from

Conversation

PopDaph
Copy link
Contributor

@PopDaph PopDaph commented Sep 7, 2023

This PR implements createAgentConfiguration.
Review with hidden whitespaces!

A few other side-changes:

  • We decided to not handle dataSourcesConfig = all
  • Timeframes are handled at the Action level so removed from AgentDataSourcesConfig

@PopDaph PopDaph requested review from spolu and fontanierh September 7, 2023 12:41
@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch from 8974e81 to ad3f29a Compare September 7, 2023 12:50
Copy link
Contributor

@spolu spolu left a 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/actions/retrieval.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent.ts Outdated Show resolved Hide resolved
name: name,
pictureUrl: pictureUrl ?? null,
scope: "workspace",
workspaceId: owner.id,
Copy link
Contributor

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 Show resolved Hide resolved
front/lib/api/assistant/agent.ts Outdated Show resolved Hide resolved
front/types/assistant/actions/retrieval.ts Outdated Show resolved Hide resolved
}
}

return _getAgentConfigurationType({
Copy link
Contributor

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?

front/types/assistant/agent-utils.ts Outdated Show resolved Hide resolved
front/types/assistant/agent-utils.ts Outdated Show resolved Hide resolved
@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch 2 times, most recently from 9551c37 to 335ab51 Compare September 7, 2023 17:11
front/lib/api/assistant/agent.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent.ts Outdated Show resolved Hide resolved
@spolu
Copy link
Contributor

spolu commented Sep 8, 2023

Note: conflicts with main prolly needs rebasing as well :)

@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch from 4cec4b9 to d2f9d8e Compare September 8, 2023 09:26
@PopDaph PopDaph requested a review from spolu September 8, 2023 09:28
front/lib/api/assistant/agent/agent_create.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_create.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_create.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_create.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_get.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_get.ts Outdated Show resolved Hide resolved
front/types/assistant/actions/retrieval.ts Outdated Show resolved Hide resolved
front/types/assistant/agent.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_get.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_update.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_update.ts Outdated Show resolved Hide resolved
front/lib/api/assistant/agent/agent_update.ts Outdated Show resolved Hide resolved
@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch 4 times, most recently from 8157e78 to 72ba099 Compare September 8, 2023 12:33
@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch from 72ba099 to b20d025 Compare September 8, 2023 12:48
(arg as TimeFrame).duration !== undefined &&
(arg as TimeFrame).unit !== undefined
);
}

// Retrieval specifies a list of data sources (with possible parent / tags filtering, possible "all"
Copy link
Contributor

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,
Copy link
Contributor

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(
Copy link
Contributor

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 ?

front/lib/models/assistant/configuration.ts Outdated Show resolved Hide resolved
front/types/assistant/configuration.ts Outdated Show resolved Hide resolved
@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch 2 times, most recently from 4b0e5cd to 90a0e85 Compare September 8, 2023 13:52
@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch from 90a0e85 to b49e22d Compare September 8, 2023 13:53
/**
* Action > Retrieval
*/
// Each AgentActionConfigurationType is capable of generating this type at runtime to specify which
Copy link
Contributor

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 = {
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Contributor

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;
Copy link
Contributor

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

Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be here?

@PopDaph PopDaph force-pushed the assistant-lib-create-agent-config branch from add5cbc to 32afccc Compare September 8, 2023 15:21
@PopDaph
Copy link
Contributor Author

PopDaph commented Sep 8, 2023

I'd rather pick from this one to create smaller PRs!

@PopDaph
Copy link
Contributor Author

PopDaph commented Sep 8, 2023

Closing and reopening smaller PRs! 👍🏻

@PopDaph PopDaph closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants