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

Add multi-language system prompts and BedrockChatAdapter implementation #576

Merged
merged 13 commits into from
Oct 31, 2024

Conversation

michel-heon
Copy link
Contributor

Pull Request: Centralize and Internationalize System Prompts

This pull request addresses the issue of scattered system prompts across the codebase and the lack of support for internationalization, as described in the corresponding Git issue #571.


Changes:

  • Commit: 256279db811d17f6c558ccf469bfbec0e0d93583
  • Objective: Centralize system prompt management and enable internationalization (i18n).
  • Affected Files:
    • system_prompts.py: A new module created to centralize all system prompts and support multiple languages (English and Canadian French).
    • base.py: Refactored methods (get_prompt, get_condense_question_prompt, get_qa_prompt) to retrieve prompts from system_prompts.py.
    • __init__.py: Updated to import system prompts for simplified access.

Key Improvements:

  • Centralization: All system prompts are now stored in one place (system_prompts.py), improving manageability and scalability.
  • Internationalization: The system can now support multiple languages, with English and Canadian French prompts available as an initial setup. This framework allows for future expansion into other languages.
  • Adapter Compatibility: Adapters such as azure-openai, mistral, claude, titan, and llama are updated to use the new prompt management system, ensuring consistency and reducing code duplication.

Testing Instructions:

  1. Deploy the Application: Follow the deployment instructions provided in the project documentation to deploy the application.
  2. Target Language: The language used by the system prompts is now managed by the mechanism in system_prompts.py.
  3. Modify Language: You can modify the target language at runtime by editing the language selection in the GenAIChatBotStack-LangchainInterfaceReques Lambda function.
  4. Prompt Tracing: The system prompts are logged and can be traced in CloudWatch. Additionally, they are available in the prompt field of the metadata variable in the AWS GenAI Chatbot console for further analysis.

Expected Outcome:

  • Simplified prompt management across adapters.
  • Enhanced scalability and flexibility in adding new languages.

- Implement system prompts in English and Canadian French for AI
interactions in `system_prompts.py`.
- Enhance `BedrockChatAdapter` with prompt templates for QA,
conversation, and follow-up questions in `base.py`.
- Update `__init__.py` to include system prompt imports for easy access.
- Configure logger in `base.py` to trace key operations for QA prompts
and conversational prompts.
Copy link
Collaborator

@charles-marion charles-marion left a comment

Choose a reason for hiding this comment

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

Thank you for creation this PR! I think it's a good addition to the project (And sorry for the delay)

The build process failed. I recommend to run npm run-vetall

"Si vous ne trouvez pas la réponse dans les documents, informez l'utilisateur que l'information n'est pas disponible. "
"Si possible, dressez la liste des documents référencés.",
# Prompt for conversational interaction between a human and AI (French-Canadian)
'conversation_prompt': "Vous êtes un assistant IA utilisant la Génération Augmentée par Récupération (RAG). "
Copy link
Collaborator

Choose a reason for hiding this comment

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

This prompt is used when RAG is not used. I think this prompt should be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is modified according to the specification

Copy link
Collaborator

Choose a reason for hiding this comment

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

@michel-heon Can you clarify what specification you are referring to?

My point is this prompt is a copy/similar to the one above qa_prompt used when a workspace is selected suggesting to use documents and RAG (but this prompt is used when no workspace is set, so no documents)

For reference this is the english version: The following is a friendly conversation between a human and an AI. " "If the AI does not know the answer to a question, it truthfully says it does not know.

It is used here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I misunderstood. I've changed the prompt to reflect the comment.

'conversation_prompt': "The following is a friendly conversation between a human and an AI. "
"If the AI does not know the answer to a question, it truthfully says it does not know.",
# Prompt for rephrasing a follow-up question to be a standalone question
'condense_question_prompt': "Given the conversation inside the tags <conv></conv>, rephrase the follow up question inside <followup></followup> to be a standalone question.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could merge condense_question_prompt and contextualize_q_system_prompt since they have the same goal. (The later is only used by bedrock)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree

# Add other languages here if needed

# Set default language (English)
lang = Language.ENGLISH.value # Default language is set to English
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend adding this as a selection option of the cli and pass it as an env variable

Could be added later and maybe also a documentation page explaining how to add languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm all for thinking things through, as both solutions offer their own list of advantages and disadvantages. I propose to create a new issue for this feature after this PR is closed.

template updates;
- improve prompt system for multilingual support;
- expand test coverage for Bedrock adapters with guardrail integration.
@charles-marion
Copy link
Collaborator

@michel-heon please tag me or click the re-request review button if you'd like me to have a look.
(Note I noticed the file base.py_new which is probably not relevant?)

@michel-heon
Copy link
Contributor Author

@charles-marion In fact, the base.py_new file is an error that I've deleted. And indeed, the code is ready for revision.

Copy link
Collaborator

@charles-marion charles-marion left a comment

Choose a reason for hiding this comment

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

Thank you for the update.
Note that I appreciate the help with this change. I can address my comments and complete the PR if you'd like (I don't want to take too much of your time)

model=self.model_id,
metric_type="token_usage",
value=self.callback_handler.usage.get("total_tokens"),
extra={
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing the JSON format here would break the metric in the dashboard. Please undo
https://github.com/aws-samples/aws-genai-llm-chatbot/blob/main/lib/monitoring/index.ts#L289


class Mode(Enum):
CHAIN = "chain"


def get_guardrails() -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is only applicable for bedrock. Why did you add it here?

@@ -342,3 +362,245 @@ def run(self, prompt, workspace_id=None, *args, **kwargs):
return self.run_with_chain(prompt, workspace_id)

raise ValueError(f"unknown mode {self._mode}")


class BedrockChatAdapter(ModelAdapter):
Copy link
Collaborator

Choose a reason for hiding this comment

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

return {
"guardrailIdentifier": os.environ["BEDROCK_GUARDRAILS_ID"],
"guardrailVersion": os.environ.get("BEDROCK_GUARDRAILS_VERSION", "DRAFT"),
}
logger.info("No guardrails ID found.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logger.info("No guardrails ID found.")
logger.debug("No guardrails ID found.")

Otherwise it will be logged on every llm call.

top_p = model_kwargs.get("topP")
max_tokens = model_kwargs.get("maxTokens")

if temperature:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would not set the value if temperature is 0

Suggested change
if temperature:
if temperature is not None:

Standalone question:""" # noqa: E501
return PromptTemplateWithHistory(
template=template, input_variables=["input", "chat_history"]
# Change le niveau global à DEBUG
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Change le niveau global à DEBUG

# Change le niveau global à DEBUG
# Fetch the prompt and translated words based on the current language
condense_question_prompt = prompts[locale]["condense_question_prompt"]
logger.info(f"condense_question_prompt: {condense_question_prompt}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logger.info(f"condense_question_prompt: {condense_question_prompt}")
logger.debug(f"condense_question_prompt: {condense_question_prompt}")

I would recommend to mark them all as debug to reduce cloudwatch usage. (nitpick sorry)

Comment on lines 36 to 37
# Setting programmatic log level
# logger.setLevel("DEBUG")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Setting programmatic log level
# logger.setLevel("DEBUG")

I would remove this because there is already a global log level setting here
https://github.com/aws-samples/aws-genai-llm-chatbot/blob/main/lib/shared/index.ts#L52

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to include this information in the developer's guide documentation?

enhanced logging for BedrockChatAdapter initialization, and streamlined
QA prompts. Removed redundant base.py_new and ensured BedrockChatAdapter
configuration is aligned with main branch consistency.
@michel-heon
Copy link
Contributor Author

I've just finished making the corrections and pushed the updated changes. No worries about the time involved—it's genuinely a pleasure to contribute to this effort.

Copy link
Collaborator

@charles-marion charles-marion left a comment

Choose a reason for hiding this comment

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

Thank you for your help with this change!.

I will merge it later this week.

@michel-heon
Copy link
Contributor Author

The i18n mechanism works for Bedrock, but not for azureopenai. This fix could be part of a future PR?

@charles-marion
Copy link
Collaborator

charles-marion commented Oct 29, 2024

The i18n mechanism works for Bedrock, but not for azureopenai. This fix could be part of a future PR?

Do you mean it breaks the azureopenai flow or just always use english prompts?
Yes it could be a future PR if it does not break capabilities.

Happy to merge today/tomorrow if that's not the case.

@michel-heon
Copy link
Contributor Author

In fact, the use of azureopenai refers to the default system-prompt in langchain, which must be overloaded. The same applies to Mistral. I agree with deferring this development to another PR and merging the current work.

@charles-marion
Copy link
Collaborator

charles-marion commented Oct 30, 2024

Build is blocked until the following is merged. #598
I will merge this PR when the above is merged.

Copy link
Collaborator

@charles-marion charles-marion left a comment

Choose a reason for hiding this comment

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

I ran the integration tests and fixed the formatting.

LGTM. Thank you for you contribution!

@charles-marion charles-marion merged commit 6a18d87 into aws-samples:main Oct 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants