Skip to content

assist: move embeddings in a separate library, add tool config, opportunistic node lookup#29716

Merged
hugoShaka merged 6 commits intomasterfrom
hugo/assist-few-nodes-bypass-embeddings
Aug 9, 2023
Merged

assist: move embeddings in a separate library, add tool config, opportunistic node lookup#29716
hugoShaka merged 6 commits intomasterfrom
hugo/assist-few-nodes-bypass-embeddings

Conversation

@hugoShaka
Copy link
Copy Markdown
Contributor

@hugoShaka hugoShaka commented Jul 27, 2023

This PR mitigates https://github.com/gravitational/teleport.e/issues/1820.

On the vast majority of Teleport Team clusters, the user only has access to a few nodes, and the cluster is small. In such cases, we can bypass the embedding lookup and feed the model with all allowed nodes directly from the proxy node watcher's cache. This allows users who just added a node to be able to use it with Assist. Assist behaviour will degrade to what we know today on larger clusters: with up to 20min of latency due to the embedding process.

This PR also adds 2 things:

  • refactor the lib/ai module to extract embedding types into lib/ai/embedding. This was made to break a cyclic dependency between lib/services and lib/ai/model. (lib/ai/model needs accessChecker from lib/services lib/services needed Embedding from lib/ai, and lib/ai depends a lot on lib/ai/model)
  • refactor the Agent to add a ToolsConfig structure allowing to configure tools and send all the clients they need while reducing the number of parameters needed to create a new Agent.

@hugoShaka hugoShaka force-pushed the hugo/assist-few-nodes-bypass-embeddings branch 4 times, most recently from 36e399f to 819e0d0 Compare July 28, 2023 15:29
This commit does two things:
- Introduce a new toolsConfig structure to pass all required clients to
  the tools. This can also be used to enable or disable specicifc tools,
  which helps with testing
- Pass the proxy node cache and the request access checker to the
  embedding tool so it can try to lookup nodes from cache in if the user
  has less than 10 nodes available.
@hugoShaka hugoShaka force-pushed the hugo/assist-few-nodes-bypass-embeddings branch from 819e0d0 to aa4b12a Compare July 28, 2023 15:43
@hugoShaka hugoShaka requested review from jakule and xacrimon July 28, 2023 15:43
@hugoShaka hugoShaka marked this pull request as ready for review July 28, 2023 15:43
Comment thread lib/ai/model/agent.go Outdated
Comment thread lib/ai/model/agent.go

tools := []Tool{&commandExecutionTool{}}

if !config.DisableEmbeddingsTool {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks a bit hacky. Now if you pass DisableEmbeddingsTool you also must pass username which won't be used.

Comment thread lib/ai/chat_test.go
},
},
want: 721,
want: 610,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The values here should not change. I think that because now the embedder is optional is disabled as a tool which is not correct. We still need to include the "get available node" tools in the prompt. We just need to use a different implementation for fetching the nodes.

Copy link
Copy Markdown
Contributor Author

@hugoShaka hugoShaka Jul 28, 2023

Choose a reason for hiding this comment

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

The test was previously instantiating a tool that would panic if invoked. As embedding tool was not used for the token counting test it did not crash.

I'm not sure what we want to test here except the tokens are counted, but IMO it is saner to remove broken tools from the test. If a test really depends on this tool, it should make it available and pass valid configuration.

Copy link
Copy Markdown
Contributor Author

@hugoShaka hugoShaka Jul 28, 2023

Choose a reason for hiding this comment

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

We'll have valid use-cases to enable/disable tools. e.g. Athena query building can be dropped if the user doesn't have access to audit logs. I think a modular agent toolbox is a pattern we'll benefit from, especially if we introduce new tools but want to reduce the token footprint.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do understand that, but then you may end up with zero tools and very bad UX. I think that saving tokens + not confusing LLM are good arguments, but we should be smarter about when each tool is available. Can we for now still require the node embedded and fix the situation later (in a proper way)?

Copy link
Copy Markdown
Contributor Author

@hugoShaka hugoShaka Aug 7, 2023

Choose a reason for hiding this comment

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

I get what you're saying and have a proposal: would you be happy if, instead of having a hacky configuration that can enable/disable tools arbitrarily for the sake of testing, we had an interface with a method that returns the tools. This way we could have a standard implementation for real invocations, and tests could define their own custom tools. Like

type Toolbox interface {
        CheckAndSetDefaults()
        GetTools(context.Context) []model.Tool
}

type DefaultToolbox struct {
        EmbeddingsClient assist.AssistEmbeddingServiceClient
	AccessChecker services.AccessChecker
	NodeClient *services.NodeWatcher
}

// Returns [executionTool, embeddingTool] and all our future tools
func (tb *DefaultToolbox) GetTools(ctx context.Context) []model.Tool {...}

And in tests we could do

type dummyToolbox struct {
    testTools []model.Tools
}

// Returns what dummy or real tools contained in the toolbox
func (tb *dummyToolbox) GetTools(ctx context.Context) []model.Tool

This would allow tests to safely choose which tools are here, and mock tools or use real tools when needed, while keeping the "real" toolbox safe and always containing all tools.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the idea of separating the tools from the agent itself. I think we will need that anyways, as when I was working on the "in-context SSH Assist" I needed a different agent with a completely different set of tools.
I think we could even rename the DefaultToolbox to ChatToolbox as it contains all tools required by the chat. Then as you mentioned, other agents (like tests) can implement a different set of available tools.
@xacrimon It this change going to conflict with your access request changes? If yes, we can ship it after this and your PR are merged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, im refactoring that area a little, sounds good.

Comment thread lib/ai/model/tool.go
Copy link
Copy Markdown
Contributor

@jakule jakule left a comment

Choose a reason for hiding this comment

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

As we talked at the meeting. The code overall looks good. The agent logic needs to change anyways to accommodate other Assisst-related changes. It can be done in a separate PR.

Comment thread lib/ai/client.go Outdated
@hugoShaka hugoShaka enabled auto-merge August 9, 2023 18:02
@hugoShaka hugoShaka added this pull request to the merge queue Aug 9, 2023
Merged via the queue into master with commit 8dbd3c3 Aug 9, 2023
@hugoShaka hugoShaka deleted the hugo/assist-few-nodes-bypass-embeddings branch August 9, 2023 18:38
jakule pushed a commit that referenced this pull request Aug 21, 2023
…tunistic node lookup (#29716)

* assist: extract embedding types into a dedicated module

* assist: allow the embedding tool to query nodes from proxy cache

This commit does two things:
- Introduce a new toolsConfig structure to pass all required clients to
  the tools. This can also be used to enable or disable specicifc tools,
  which helps with testing
- Pass the proxy node cache and the request access checker to the
  embedding tool so it can try to lookup nodes from cache in if the user
  has less than 10 nodes available.

* Add tests for the opportunistic proxy node lookup

* Apply suggestions from code review

* Update lib/ai/client.go
github-merge-queue Bot pushed a commit that referenced this pull request Aug 22, 2023
* assist: move embeddings in a separate library, add tool config, opportunistic node lookup (#29716)

* assist: extract embedding types into a dedicated module

* assist: allow the embedding tool to query nodes from proxy cache

This commit does two things:
- Introduce a new toolsConfig structure to pass all required clients to
  the tools. This can also be used to enable or disable specicifc tools,
  which helps with testing
- Pass the proxy node cache and the request access checker to the
  embedding tool so it can try to lookup nodes from cache in if the user
  has less than 10 nodes available.

* Add tests for the opportunistic proxy node lookup

* Apply suggestions from code review

* Update lib/ai/client.go

* [Assist] Add Assist to the terminal UI (#30496)

* Add Assist to the terminal UI

* Use getPlatform()

* Fix spacing beneath the message box

* [Assist] Add in SSH context Assist endpoints (#30319)

* Add in SSH context Assist endpoints

This change introduces web endpoints used by Assist to support in SSH context functionality in the WebUI. Now a user will be able to generate Bash command and explain output in the terminal (like application logs).

* Remove mockup web UI

* Make the linter happy

* Remove unnecessary comments and add missing documentation

This commit removes a redundant comment on the AI model selection in client.go that suggested a model change that is not needed. It also adds missing documentation in chat.go, to clarify the purpose and functionality of the Reply function.

* Revert ws.WriteControl to use real time.

Usage of the fake clock was failing the tests, and it's not really beneficial in this case.

* Replace caret symbol with Windows key in Terminal utility (#30642)

The caret symbol (^) previously used in the Terminal utility was not universally recognized or understanding. To increase usability, it has been replaced with the Windows key symbol (⊞). This should make the tool more intuitive to use for a wider range of users.

* [Assist] Add AssistAction and AssistAccessRequest PostHog event reporting (#30513)

* Add AssistAction and AssistAccessRequest PostHog event reporting

This commit introduced new usage tracking for Assist features. AssistAction event is emitted when user triggers an action like SSH command generation or output explaining. AssistAccessRequest event is fired when user requests access to a resource. The events capture anonymized username, resource type or action triggered, and a breakdown of tokens used for generating command summaries or completion responses.

* "Removed userName in AssistAccessRequest and AssistAction"

Both "AssistAccessRequest" and "AssistAction" messages in usageevents.proto file and their corresponding Go structures were modified to remove the "userName" field.

* Add GoDoc

---------

Co-authored-by: Hugo Shaka <hugo.hervieux@goteleport.com>
Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants