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

chore(bedrock): agent refactoring #833

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

aws-rafams
Copy link
Contributor

@aws-rafams aws-rafams commented Dec 3, 2024

Fixes #731, #767, #747

  • Interfaces
  • Unit Testing
  • Integ Testing

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@aws-rafams aws-rafams requested a review from a team as a code owner December 3, 2024 14:13
@aws-rafams aws-rafams marked this pull request as draft December 3, 2024 14:13
@aws-rafams aws-rafams changed the title refactor(bedrock): Agent refactoring refactor(bedrock): agent refactoring Dec 3, 2024
@aws-rafams aws-rafams changed the title refactor(bedrock): agent refactoring chore(bedrock): agent refactoring Dec 3, 2024
@krokoko
Copy link
Collaborator

krokoko commented Dec 10, 2024

Upgraded the bedrock agent sample with these changes, first notes:

  • [FIXED] Bedrock KB Instruction prop became instructionForAgents breaking change
    I think we can revert that to instruction

  • Enable user input -> breaking change

const agent = new bedrock.Agent(**this**, 'Agent', {

      foundationModel: bedrock.BedrockFoundationModel.ANTHROPIC_CLAUDE_INSTANT_V1_2,

      instruction: 'You are a helpful and friendly agent that answers questions about literature.',

      knowledgeBases: [kb],

      enableUserInput: true,

      shouldPrepareAgent:true

    });
  • [FIXED] AgentActionGroup name changed to ActionGroup (breaking change), however the Cfn resource is an AgentActionGroup

  • apiSchema: bedrock.ApiSchema.fromAsset(path.join(__dirname, 'action-group.yaml')), changed to fromLocalAsset -> breaking change

  • [FIXED] addActionGroups -> api removed, I think we can bring this back since it's convenient

  • addAlias method on agent -> disappeared, breaking change

  • "ActionGroupState" on agent action group became “enable” -> breaking change

  • CDK nag: AwsSolutions-IAM5[Resource::<ActionGroupFunctionFE14D1CB.Arn>:*]: The IAM entity contains wildcard permissions and does not have a cdk-nag rule suppression with evidence for those permission.

@krokoko
Copy link
Collaborator

krokoko commented Dec 10, 2024

After updating the sample to work with those changes:

  • [FIXED] User input seems to not be correctly configured in the UI (action group user input present and enabled, but user input is checked as disabled in the console):

image

This is a regression, it is correctly configured with the existing code sample:

image

  • Default model Claude instant v1.2 fails at deploying with the following error: Resource handler returned message: "CreateAgentActionGroupRequest caught an error. Unsupported Foundation Model: anthropic.claude-instant-v1 (Service: BedrockAgent , Status Code: 400, Request ID: 13224dfc-a646-492c-bf89-7f1d4a235b7c)" (RequestToken: b46afff2-f763-2008-7a9d-4c747fa40aaa, HandlerErrorCode: GeneralServiceExcepti on) -> it deploys successfully with the existing code sample, so this is a regression

  • Running a request produces the following error: 1 validation error detected: Value 'GET__query-library__getTopBooks' at 'toolConfig.tools.1.member.toolSpec.name' failed to satisfy constraint: Member must satisfy regular expression pattern: [a-zA-Z][a-zA-Z0-9_]* -> regression as it is working with the existing code sample

@krokoko
Copy link
Collaborator

krokoko commented Dec 10, 2024

In addition to the scenarios mentioned in the PR listed tickets, we need to test/verify:

  • Bedrock Agent Sample works as expected (tested above, not working for now)
  • Use the sample above and add guardrails
  • Override prompts
  • Test customControl in ActionGroupExecutor
  • Agent aliases
  • Load api schema from S3 and inline
  • CRIS
  • TS and Python tests

@krokoko
Copy link
Collaborator

krokoko commented Dec 12, 2024

Testing to override default prompt for an agent:

const prompt_path = path.join(__dirname, 'pre-processing.txt');

    const file = readFileSync(prompt_path, 'utf-8');
    
    const agent = new bedrock.Agent(this, 'Agent', {
      foundationModel: bedrock.BedrockFoundationModel.AMAZON_NOVA_LITE_V1,
      instruction: 'You are a helpful and friendly agent that answers questions about literature.',
      userInputEnabled: true,
      codeInterpreterEnabled: false,
      shouldPrepareAgent:true,
      promptOverrideConfiguration: bedrock.PromptOverrideConfiguration.fromSteps(
        [
          {
            stepType: bedrock.AgentStepType.PRE_PROCESSING,
            stepEnabled: true,
            customPromptTemplate: file,
            inferenceConfig: {
              temperature: 0.0,
              topP: 1,
              topK: 250,
              maximumLength: 1,
              stopSequences: ["\n\nHuman:"],
            },
          }
        ]
      )
    });

Seems to work fine:

image

Need to test more scenarios

@krokoko
Copy link
Collaborator

krokoko commented Dec 12, 2024

Adding a guardrail fails:

// create your agent

const guardrails = new bedrock.Guardrail(this, "bedrockGuardrails", {
      name: "my-BedrockGuardrails",
      description: "Legal ethical guardrails.",
    });

    guardrails.addDeniedTopicFilter(bedrock.Topic.FINANCIAL_ADVICE);

    agent.addGuardrail(guardrails);

synth fails with:

validation-helpers.ts:98
    throw new Error(errors.join('\n'));
          ^
Error: The field version with value "${Token[TOKEN.1058]}" does not match the required pattern /^(([0-9]{1,8})|(DRAFT))$/
    at Object.throwIfInvalid (.../src/common/helpers/validation-helpers.ts:98:11)

from this:

public addGuardrail(guardrail: IGuardrail) {
    // Do some checks
    validation.throwIfInvalid(this.validateGuardrail, guardrail);
    // Add it to the construct
    this.guardrail = guardrail;
    // Handle permissions
    guardrail.grantApply(this.role);
  }

@aws-rafams
Copy link
Contributor Author

aws-rafams commented Dec 16, 2024

Adding a guardrail fails:

// create your agent

const guardrails = new bedrock.Guardrail(this, "bedrockGuardrails", {
      name: "my-BedrockGuardrails",
      description: "Legal ethical guardrails.",
    });

    guardrails.addDeniedTopicFilter(bedrock.Topic.FINANCIAL_ADVICE);

    agent.addGuardrail(guardrails);

synth fails with:

validation-helpers.ts:98
    throw new Error(errors.join('\n'));
          ^
Error: The field version with value "${Token[TOKEN.1058]}" does not match the required pattern /^(([0-9]{1,8})|(DRAFT))$/
    at Object.throwIfInvalid (.../src/common/helpers/validation-helpers.ts:98:11)

from this:

public addGuardrail(guardrail: IGuardrail) {
    // Do some checks
    validation.throwIfInvalid(this.validateGuardrail, guardrail);
    // Add it to the construct
    this.guardrail = guardrail;
    // Handle permissions
    guardrail.grantApply(this.role);
  }

I see, validation occurs even when tokens are unresolved. Will modify and add the !cdk.Token.isUnresolved(guardrailName) part missing

@dineshSajwan
Copy link
Contributor

Tested Python samples with the changes and it worked all fine.
Screenshot 2024-12-19 at 12 09 04 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(bedrock): agent module refactoring to follow best practices from CDK
4 participants