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

Bedrock basic answer strategy #27

Merged
merged 5 commits into from
Feb 6, 2025
Merged

Conversation

chaecramb
Copy link
Contributor

@chaecramb chaecramb commented Jan 29, 2025

This PR implements a basic answer strategy using Claude 3.5 Sonnet on Bedrock. In this initial implementation we simply have Claude provide a structured JSON response directly, without rephrasing, guardrails or RAG.

It does four things:

  • Allow configuration of answer strategy via environment variables.
  • Convert the OpenAIAnswer class to a more generic PipelineRunner class, this can be used to run the pipeline for both Claude and OpenAI.
  • Creates a StructuredAnswerComposer for Claude that returns a JSON response via a tool call.
  • Sets up and implements initial unit and system tests for Claude, along with relevant helpers.

Trello: https://trello.com/c/wB5dUyQ7/2257-integrate-a-very-basic-answer-strategy-no-rag-with-aws-bedrock

@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-lyndt4 January 29, 2025 16:24 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 26279fd to c75e7f0 Compare January 30, 2025 10:22
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-p4nmw9 January 30, 2025 10:26 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from c75e7f0 to 6442a60 Compare January 30, 2025 10:48
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ptaxdm January 30, 2025 10:52 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 6442a60 to e1bd956 Compare January 30, 2025 12:12
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-2vaati January 30, 2025 12:16 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from f1cc88e to 09bdd47 Compare January 30, 2025 15:54
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-dnhwkc January 30, 2025 15:55 Inactive
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks great Chae, sorry for taking a while to get this review together.

I think the other thing to add which we discussed was a system test similar to https://github.com/alphagov/govuk-chat/blob/main/spec/system/conversation_with_open_ai_with_structured_answer_spec.rb

@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 09bdd47 to cdd74c1 Compare January 30, 2025 16:56
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-4y42rx January 30, 2025 16:58 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from cdd74c1 to e7a2a7e Compare January 31, 2025 14:27
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-6imgbo January 31, 2025 14:32 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-z3hz24 January 31, 2025 15:47 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 7589e94 to 1cd13cb Compare January 31, 2025 16:26
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-oecebs January 31, 2025 16:28 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 1cd13cb to e824b5c Compare January 31, 2025 16:31
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-idsu5w January 31, 2025 16:35 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from e824b5c to f166e5f Compare February 5, 2025 09:49
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 5, 2025 09:54 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 5, 2025 10:39 Inactive
@kevindew kevindew force-pushed the bedrock-basic-answer-strat branch from 1fe111c to 6b5c3a7 Compare February 5, 2025 10:41
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 5, 2025 10:41 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 6b5c3a7 to df027e7 Compare February 5, 2025 12:41
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 5, 2025 12:41 Inactive
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from df027e7 to 54fb7ce Compare February 5, 2025 15:08
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 5, 2025 15:09 Inactive
@chaecramb chaecramb marked this pull request as ready for review February 5, 2025 15:16
@chaecramb chaecramb changed the title WIP: Bedrock basic answer strategy Bedrock basic answer strategy Feb 5, 2025
Changes the approach to setting the answer strategy when a new
Question is created to now be configured via an env var. Currently
this defaults to `openai_structured_answer`, but allows for the
creation of an `ANSWER_STRATEGY` variable in `.env` which can be set
to `"claude_structured_answer"` in order to switch to using Claude
via Bedrock as an answer service.
The `OpenAIAnswer` class has minimal code that is specific to OpenAI
so has been renamed to the more generic `PipelineRunner`. This class
will be used to execute the pipeline regardless of which answer
service is configured by the answer strategy.

Currently catches errors separately for both OpenAI and AWS, but
these may be abstracted in the future.
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 54fb7ce to f90e696 Compare February 5, 2025 15:34
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 5, 2025 15:34 Inactive
Copy link
Contributor

@jackbot jackbot left a comment

Choose a reason for hiding this comment

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

Couple of little comments but looks great to me.

Creates a `StructuredAnswerComposer` class for Claude that makes
use of the AWS BedrockRuntime SDK to use Claude as an answer
service.

In order to force a JSON output from Claude it uses a tool call,
this is configured to always be used via the `tool_choice`
configuration option for the Bedrock request.
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from f90e696 to 6271202 Compare February 6, 2025 10:29
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 6, 2025 10:30 Inactive
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks great, just made a suggestion to the stub_bedrock comment to clear it up then good to merge.

I've tried it out locally and it works great.

This sets up testing for the Claude structured answer composer.

It creates an initial set of tests. It also creates a set of helpers
in StubBedrock. The helpers include a general task to stub the Bedrock
converse endpoint, and then high and low level tasks for stubbing the
Claude tool call responses.
This creates system tests for a conversation with Claude using
structured answers.

It also adds a new system spec helper to allow a scenario where
Rails configuration is set to use Claude as an answer strategy.
@chaecramb chaecramb force-pushed the bedrock-basic-answer-strat branch from 6271202 to 40b1146 Compare February 6, 2025 11:28
@govuk-ci govuk-ci temporarily deployed to govuk-chat-bedrock-basi-ahpivi February 6, 2025 11:28 Inactive
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Great, let's go 🚀

@chaecramb chaecramb merged commit 6fc80a5 into main Feb 6, 2025
8 checks passed
@chaecramb chaecramb deleted the bedrock-basic-answer-strat branch February 6, 2025 11:36
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.

4 participants