-
Notifications
You must be signed in to change notification settings - Fork 86
feat(templates): wire in memory to the runtime templates #2116
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
Changes from all commits
93ffe4b
ffa2121
13f6e95
7509cd8
dfa35c5
8355682
3c10f37
1ff939b
763be27
f6d83ca
970ecac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import os | ||
| import uuid | ||
| from typing import Optional | ||
|
|
||
| from bedrock_agentcore.memory.integrations.strands.config import AgentCoreMemoryConfig{{#if memoryStrategies.length}}, RetrievalConfig{{/if}} | ||
| from bedrock_agentcore.memory.integrations.strands.session_manager import AgentCoreMemorySessionManager | ||
|
|
||
| MEMORY_ID = os.getenv("{{memoryEnvVarName}}") | ||
| REGION = os.getenv("AWS_REGION") | ||
|
|
||
|
|
||
| def get_memory_session_manager( | ||
| session_id: Optional[str], actor_id: str | ||
| ) -> Optional[AgentCoreMemorySessionManager]: | ||
| if not MEMORY_ID: | ||
| return None | ||
|
|
||
| session_id = session_id or uuid.uuid4().hex | ||
|
|
||
| {{#if memoryStrategies.length}} | ||
| retrieval_config = { | ||
| {{#if (includes memoryStrategies "SEMANTIC")}} | ||
| f"/users/{actor_id}/facts": RetrievalConfig(top_k=3, relevance_score=0.5), | ||
| {{/if}} | ||
| {{#if (includes memoryStrategies "USER_PREFERENCE")}} | ||
| f"/users/{actor_id}/preferences": RetrievalConfig(top_k=3, relevance_score=0.5), | ||
| {{/if}} | ||
| {{#if (includes memoryStrategies "EPISODIC")}} | ||
| f"/episodes/{actor_id}/{session_id}": RetrievalConfig(top_k=5, relevance_score=0.5), | ||
| {{/if}} | ||
| {{#if (includes memoryStrategies "SUMMARIZATION")}} | ||
| f"/summaries/{actor_id}": RetrievalConfig(top_k=3, relevance_score=0.5), | ||
| {{/if}} | ||
| } | ||
| {{/if}} | ||
|
|
||
| return AgentCoreMemorySessionManager( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine for now since we said we don't want to make to many changes, and I know you said you would eventually like to improve the template. One thing we should change is to use the new AgentCoreMemoryManager and AgentCoreMemoryStore at some point. We should theoretically be using our own best practices.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, good callout. |
||
| AgentCoreMemoryConfig( | ||
| memory_id=MEMORY_ID, | ||
| session_id=session_id, | ||
| actor_id=actor_id, | ||
| {{#if memoryStrategies.length}} | ||
| retrieval_config=retrieval_config, | ||
| {{/if}} | ||
| ), | ||
| REGION, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,18 +71,30 @@ export class FsTreeNode { | |
| } | ||
|
|
||
| /** | ||
| * Expands the flat asset listing under assetDir into a nested tree of nodes. | ||
| * Builds a file tree from assets under `input.assetDir`. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now thats a code comment! |
||
| * | ||
| * @param config - Asset source configuration. | ||
| * @param input - Asset directory to load. | ||
| * @param options - Optional root name, lazy content transform, and descendant filter. Rejecting a directory omits its subtree. | ||
| */ | ||
| static async fromAssetSource( | ||
| src: AssetSource, | ||
| assetDir: string, | ||
| rootDirName?: string, | ||
| transform?: (content: string) => string, | ||
| config: { assetSource: AssetSource }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like how we are using object types here. |
||
| input: { assetDir: string }, | ||
| options?: { | ||
| rootDirName?: string; | ||
| transformContent?: (content: string) => string; | ||
| filter?: (name: string, isDir: boolean) => boolean; | ||
| }, | ||
| ): Promise<FsTreeNode> { | ||
| const paths = await src.list(assetDir); | ||
| const { assetSource } = config; | ||
| const { assetDir } = input; | ||
| const rootDirName = options?.rootDirName; | ||
| const transformContent = options?.transformContent; | ||
| const filter = options?.filter; | ||
| const paths = await assetSource.list(assetDir); | ||
| const root = FsTreeNode.createDirectory(rootDirName ?? assetDir, []); | ||
|
|
||
| for (const assetPath of paths) { | ||
| assetPaths: for (const assetPath of paths) { | ||
| const relative = assetPath.slice(assetDir.length + 1); | ||
| const segments = relative.split("/"); | ||
| if (segments.some((s) => s === "" || s === "." || s === "..")) { | ||
|
|
@@ -92,25 +104,31 @@ export class FsTreeNode { | |
| } | ||
|
|
||
| let parent = root; | ||
| segments.forEach((segment, index) => { | ||
| if (index === segments.length - 1) { | ||
| for (const [index, segment] of segments.entries()) { | ||
| const isDir = index < segments.length - 1; | ||
| const name = isDir ? segment : renderName(segment); | ||
| // if the segment of a path rejects, reject the rest of the path so we jump to top-loop via assetPaths label. | ||
| if (filter && !filter(name, isDir)) continue assetPaths; | ||
|
|
||
| if (!isDir) { | ||
| parent.children.push( | ||
| FsTreeNode.createFile(renderName(segment), async () => { | ||
| const raw = await src.read(assetPath); | ||
| return transform ? transform(raw) : raw; | ||
| FsTreeNode.createFile(name, async () => { | ||
| const raw = await assetSource.read(assetPath); | ||
| return transformContent ? transformContent(raw) : raw; | ||
| }), | ||
| ); | ||
| return; | ||
| continue; | ||
| } | ||
|
|
||
| let child = parent.children.find((n): n is FsTreeNode => n.isDir && n.name === segment); | ||
| let child = parent.children.find( | ||
| (node): node is FsTreeNode => node.isDir && node.name === name, | ||
| ); | ||
| if (!child) { | ||
| child = FsTreeNode.createDirectory(segment, []); | ||
| child = FsTreeNode.createDirectory(name, []); | ||
| parent.children.push(child); | ||
| } | ||
|
|
||
| parent = child; | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return root; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,12 +60,17 @@ const getTemplateResolvers = (assetSource: AssetSource, templateRenderer: Templa | |
| [buildResolverKey("none", "Python")]: async (input: RuntimeResourceConfig) => { | ||
| if (input.protocol !== undefined && input.protocol !== "HTTP") | ||
| throw new InputValidationError(`hello-world-python only supports HTTP protocol`); | ||
| if (input.scaffoldRuntimeInput.memory !== undefined) | ||
| throw new InputValidationError(`memory is not supported with the hello-world template`); | ||
| const tree = await FsTreeNode.fromAssetSource( | ||
| assetSource, | ||
| input.scaffoldRuntimeInput.build === "Container" | ||
| ? "templates/hello-world-python-container" | ||
| : "templates/hello-world-python", | ||
| input.name, | ||
| { assetSource }, | ||
| { | ||
| assetDir: | ||
| input.scaffoldRuntimeInput.build === "Container" | ||
| ? "templates/hello-world-python-container" | ||
| : "templates/hello-world-python", | ||
| }, | ||
| { rootDirName: input.name }, | ||
| ); | ||
| return { tree, spec: { runtimes: [buildRuntimeSpec(input)] } }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the custom hello-world path can silently ignore
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is indirectly fixed in #2130 (comment). Let me rebase and verify. |
||
| }, | ||
|
|
@@ -90,10 +95,14 @@ const getTemplateResolvers = (assetSource: AssetSource, templateRenderer: Templa | |
| ? [{ mountPath: configuration.s3FilesAccessPoint.mountPath }] | ||
| : [], | ||
| ); | ||
| const memory = input.scaffoldRuntimeInput.memory; | ||
| const context = { | ||
| name: toPythonPackageName(input.name), | ||
| modelProvider: input.scaffoldRuntimeInput.modelProvider, | ||
| hasMemory: input.scaffoldRuntimeInput.memory !== "none", | ||
| hasMemory: memory !== undefined, | ||
| // the CDK injects this env var corresponding to the actual ID once its resolved on deployment. | ||
| memoryEnvVarName: memory ? `MEMORY_${memory.name.toUpperCase()}_ID` : undefined, | ||
| memoryStrategies: memory?.strategies.map(({ type }) => type) ?? [], | ||
| hasIdentity: false, | ||
| hasGateway: false, | ||
| hasPayment: false, | ||
|
|
@@ -108,14 +117,20 @@ const getTemplateResolvers = (assetSource: AssetSource, templateRenderer: Templa | |
| hasConfigBundle: false, | ||
| }; | ||
| const tree = await FsTreeNode.fromAssetSource( | ||
| assetSource, | ||
| "templates/strands-http-python", | ||
| input.name, | ||
| (raw) => templateRenderer.render(raw, context), | ||
| { assetSource }, | ||
| { assetDir: "templates/strands-http-python" }, | ||
| { | ||
| rootDirName: input.name, | ||
| transformContent: (raw) => templateRenderer.render(raw, context), | ||
| filter: (name, isDir) => memory !== undefined || !isDir || name !== "memory", | ||
| }, | ||
| ); | ||
| return { | ||
| tree, | ||
| spec: { runtimes: [{ ...buildRuntimeSpec(input), protocol: "HTTP" as const }] }, | ||
| spec: { | ||
| runtimes: [{ ...buildRuntimeSpec(input), protocol: "HTTP" as const }], | ||
| ...(memory && { memories: [memory] }), | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
|
|
||
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 think this retrieval namespace is different from the strategy being created.
MEMORY_SHORTCUTSconfigures SUMMARIZATION as/summaries/{actorId}/{sessionId}, but the generated runtime queries/summaries/{actor_id}. That means summaries written under the configured session namespace will not be retrieved.I think this should probablyt include
session_id. I just looked and this mismatch also exists in the old template, but this PR makes that strategy part of the default memory so we may as well just make it right here.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 think this is actually intentional. The namespace in the
MEMORY_SHORTCUTSis where the LTM records get written (session specific path), and then the agent retrieves those records across all sessions by dropping the sessionId on the retrieval path.I see a PR from main that fixes this exact behavior: #1660.
Uh oh!
There was an error while loading. Please reload this page.
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 see, I didn't realize this. This is definitely correct I was treating the retrieval namespace like an exact match, thanks!