refactor(conversation): consolidate resolver generation logic #2957

atierian · 2024-10-15T21:33:46Z

Description of changes

Consolidates regex substitution for JS resolver templates in single place.
Moves resolvers/*.template.js files to resolvers/templates/*.template.js
Defines ResolverFunctionDefinition and PipelineDefinition types to allow function and pipeline definitions to live as configuration rather than runtime logic.
Reworks invoke-lambda function template to remove the need for string defined JS resolver blocks in runtime code.
Shuffles a few type and function definitions around for file hygiene.
Removes any relics of "session" (property names, inline documentation) for conversation model.
Sorts import statements.

CDK / CloudFormation Parameters Changed

N/A

Issue #, if available

N/A

Description of how you validated changes

CI PR workflow
E2E run

Checklist

PR description included
yarn test passes
E2E test run linked
~~Tests are changed or added~~
~~Relevant documentation is changed or added (and PR referenced)~~
~~New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies~~
~~Any CDK or CloudFormation parameter changes are called out explicitly~~

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…sageId

atierian · 2024-10-21T20:22:44Z

...ransformer/src/__tests__/__snapshots__/amplify-graphql-conversation-transformer.test.ts.snap

+    modelId: "anthropic.claude-3-haiku-20240307-v1:0",
+    systemPrompt: "You are a helpful chatbot. Answer questions to the best of your ability.",
+    inferenceConfiguration: {"temperature":0.5,"topP":0.9,"maxTokens":100},
+  };


Changes are from JSON.stringify(...) ('' --> "") and maintaining proper indentation.

atierian · 2024-10-21T20:24:38Z

...ransformer/src/__tests__/__snapshots__/amplify-graphql-conversation-transformer.test.ts.snap

+  const dataTools = undefined;
+  const toolsConfiguration = { dataTools, clientTools };


This is expected if no tools are provided in the conversation directive. It was part of reworking the resolver function generation to stop constructed fragments of the JS resolver runtime code as strings within the transformer.

atierian · 2024-10-21T20:25:21Z

packages/amplify-graphql-conversation-transformer/src/conversation-directive-configuration.ts

+/**
+ * Configuration for the Conversation Directive
+ */
+export type ConversationDirectiveConfiguration = {


Consolidating directive configuration relevant types here.

atierian · 2024-10-21T20:26:59Z

packages/amplify-graphql-conversation-transformer/src/graphql-types/name-values.ts

+import pluralize from 'pluralize';
+import { ConversationDirectiveConfiguration } from '../conversation-directive-configuration';
+
+export const CONVERSATION_MESSAGES_REFERENCE_FIELD_NAME = 'conversationId';


Consolidating strings needed in various places and often multiple times.

atierian · 2024-10-21T20:28:16Z

packages/amplify-graphql-conversation-transformer/src/tools/process-tools.ts

-export type Tools = {
-  tools: Tool[];


This type was unnecessary.

atierian · 2024-10-21T20:32:29Z

packages/amplify-graphql-conversation-transformer/src/grapqhl-conversation-transformer.ts

-  validate = (): void => {
-    for (const directive of this.directives) {
-      if (directive.field.type.kind !== 'NamedType' || directive.field.type.name.value !== 'ConversationMessage') {
-        throw new InvalidDirectiveError('@conversation return type must be ConversationMessage');
-      }
-      if (directive.handler && directive.functionName) {
-        throw new InvalidDirectiveError("'functionName' and 'handler' are mutually exclusive");
-      }
-      if (directive.handler) {
-        const eventVersion = semver.coerce(directive.handler.eventVersion);
-        if (eventVersion?.major !== 1) {
-          throw new Error(
-            `Unsupported custom conversation handler. Expected eventVersion to match 1.x, received ${directive.handler.eventVersion}`,
-          );
-        }
-      }
-    }
-  };


This validation logic was moved to conversation-field-handler.ts where existing validation logic lives.

atierian · 2024-10-21T20:34:14Z

...ify-graphql-conversation-transformer/src/resolvers/assistant-response-pipeline-definition.ts

+ */
+function data(): ResolverFunctionDefinition {
+  return createResolverFunctionDefinition({
+    slotName: 'data',


The name 'data' for this slot follows requirements set by TransformerResolver.

atierian · 2024-10-21T20:41:56Z

packages/amplify-graphql-conversation-transformer/src/resolvers/assistant-mutation-resolver.ts

These now live as ResolverFunctionDefinitions. For example, this one is the data() definition within assistant-response-pipeline-definition.ts.

packages/amplify-graphql-conversation-transformer/src/conversation-directive-configuration.ts

palpatim · 2024-10-21T20:42:19Z

packages/amplify-graphql-conversation-transformer/src/graphql-types/conversation-model.ts

 * @property {DirectiveNode} conversationAuthDirective - The auth directive for the conversation model.
- * @property {DirectiveNode} conversationModelDirective - The model directive for the conversation model.
+ * @property {DirectiveNode} conversationDirective - The model directive for the conversation model.
 * @property {DirectiveNode} conversationHasManyMessagesDirective - The has-many directive for the messages relationship.
 * @property {FieldDefinitionNode} conversationMessagesField - The field definition for the messages relationship.
- * @property {ObjectTypeDefinitionNode} conversationModel - The complete conversation model object type definition.
+ * @property {ObjectTypeDefinitionNode} conversation - The complete conversation model object type definition.


nit: doccomment names don't match field names.

nit: When I first read this I assumed this was the TS representation of the model object itself, and wasn't until I dug into it further that I realized that shape was actually not represented at all. What is this structure actually representing? The model should have the directives and fields on it -- is this just to hoist them into a dedicated property to make them easier to work with?

Thanks for catching that. Addressed in a3111dc

The properties represent GraphQL document nodes (DirectiveNode, FieldDefinitionNode, ObjectTypeDefinitionNode, etc) that are required input for APIs exposed by dependent transformers (model / relational / auth). For example, the modelDirective within ConversationModel is used when invoking modelTransformer.object. The alternative is passing around only the ObjectTypeDefinitionNode and extracting the other nodes as needed, but that leads to some gnarly and difficult to maintain code.

No strong objections, but I'll note that if we need more transformers to operate on a the conversation model, you'll have to hoist more & more properties into this model structure. I suppose it's not all that different from writing utility methods to extract same, except that you're carrying the properties around everywhere rather than invoking a utility method when needed. I'll chalk this up to a stylistic preference & move on. :)

packages/amplify-graphql-conversation-transformer/src/graphql-types/message-model.ts

...ify-graphql-conversation-transformer/src/resolvers/assistant-response-pipeline-definition.ts

packages/amplify-graphql-conversation-transformer/src/resolvers/generate-resolver.ts

packages/amplify-graphql-conversation-transformer/src/resolvers/index.ts

packages/amplify-graphql-conversation-transformer/src/resolvers/resolver-function-definition.ts

...s/amplify-graphql-conversation-transformer/src/resolvers/send-message-pipeline-definition.ts

...amplify-graphql-conversation-transformer/src/transformer-steps/conversation-field-handler.ts

sobolk · 2024-10-22T22:24:27Z

...amplify-graphql-conversation-transformer/src/transformer-steps/conversation-field-handler.ts

+   * @throws {InvalidDirectiveError} If the configuration is invalid.
+   */
+  private validateHandler(config: ConversationDirectiveConfiguration): void {
+    if (config.handler && config.functionName) {


we could remove functionName at this point.

Yea, I'll follow up with that. This PR is exclusively non-functional changes and considering its size, I'd like to keep it that way.

atierian added 30 commits October 1, 2024 13:30

fix IndexName -> index in otherwise unused query list resolver

30d2087

update snapshots in relational, searchable, and index transformers

b2b2135

single item messages, replace assistantContent with associatedUserMes…

11de0e3

…sageId

assistant mutation pipeline writes instead of updates

06c295a

update event payload for lambda history retrieval pattern

c8d7500

update test case for num resolver fns in list query pipeline

b3a9e19

remove straggler import statement

1170798

lint

019e01f

wip conversation-pipeline-resolver abstraction

d35bd88

wip pipeline defs

90d6131

wip add subscription pipeline definition

b75b80d

Merge branch 'main' into ai.conversation-consolidate-resolver-generation

a9df4e9

fix use correct field and type names for resolvers

dbc52bf

remove now unneeded files

175fa36

cleanup

90afcb5

consolidate datasource and substitution helpers

5162880

simplify pipeline resolver by using function rather than class

22e2b68

move templates to a subdirectory

a0062d0

organize imports

f0e51d9

move definitions types to own file

fc5d8fb

file orgnization hygiene

77da9b1

remove string template values

2acdfc1

prettier

8893a15

Merge branch 'main' into ai.conversation-consolidate-resolver-generation

ef784a5

move handler configuration into types file

288d076

file name and inline comments

60683e7

add MODEL_ID and SYSTEM_PROMPT substitutions

204019a

cleanup

5dc7752

remove unused helper in test case

5e7e0a6

lint

5f6712b

atierian commented Oct 21, 2024

View reviewed changes

add back existing functionName deprecation wording

ac1148f

palpatim previously approved these changes Oct 21, 2024

View reviewed changes

extract api

80948aa

atierian dismissed palpatim’s stale review via 80948aa October 21, 2024 22:36

atierian requested a review from a team as a code owner October 21, 2024 22:36

atierian added 4 commits October 22, 2024 09:55

fix doc comment names

a3111dc

Error -> InvalidDirectiveError

0b84777

function -> const

fb1f132

export same names from resolver submodule

3c52969

palpatim previously approved these changes Oct 22, 2024

View reviewed changes

sundersc previously approved these changes Oct 22, 2024

View reviewed changes

sobolk reviewed Oct 22, 2024

View reviewed changes

Merge branch 'main' into ai.conversation-consolidate-resolver-generation

61052fe

atierian dismissed stale reviews from sundersc and palpatim via 61052fe October 23, 2024 12:49

palpatim approved these changes Oct 23, 2024

View reviewed changes

atierian enabled auto-merge (squash) October 23, 2024 14:56

sundersc approved these changes Oct 23, 2024

View reviewed changes

atierian merged commit c85975b into main Oct 23, 2024
7 checks passed

atierian deleted the ai.conversation-consolidate-resolver-generation branch October 23, 2024 19:32

atierian mentioned this pull request Oct 24, 2024

fix(conversation): inline template for init slot and correct datasource in assistant response mutation #2980

Merged

7 tasks

tejas2008 pushed a commit that referenced this pull request Oct 29, 2024

refactor(conversation): consolidate resolver generation logic (#2957)

9730c1b

		const dataTools = undefined;
		const toolsConfiguration = { dataTools, clientTools };

refactor(conversation): consolidate resolver generation logic #2957

refactor(conversation): consolidate resolver generation logic #2957

Uh oh!

Conversation

atierian commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

CDK / CloudFormation Parameters Changed

Issue #, if available

Description of how you validated changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

atierian commented Oct 15, 2024 •

edited

Loading