AGML 154: Migrate Prompt Templating #373
Conversation
gautam-arklex
requested review from
luyunan0404 and
xinyang-articulate
as code owners
|
ToDo: Print lines need to be removed after testing before merging to main. |
arklex/models/model_service.py
Outdated
| # Format messages with system prompt if provided | ||
| messages = [] | ||
| # Print statement to show what's being sent to get_response | ||
| print("\n" + "="*80) |
There was a problem hiding this comment.
Let's us logging instead of printing
There was a problem hiding this comment.
This temporary. Will remove it after testing is done: unless you recommend keeping it. I can convert it as a logger in that case
pyproject.toml
Outdated
| "fastapi-cli>=0.0.5,<1.0.0", | ||
| "greenlet>=3.1.1,<4.0.0", | ||
| "httptools>=0.6.4,<1.0.0", | ||
| "httpx>=0.28.1,<1.0.0", |
There was a problem hiding this comment.
Is this additional package needed?
There was a problem hiding this comment.
I think this creeped in from the original PR. I'm investigating this rn
arklex/utils/prompts.py
Outdated
|
|
||
| If you provide specific details in the response, it should be based on the conversation history or context below. Do not hallucinate.""", | ||
| "context_generator_prompt": """Conversation: | ||
| {formatted_chat} |
There was a problem hiding this comment.
As discussed for the task, we would split the conversation history into format of:
[
{
"role": "system",
"message": "<system_prompt>"
},
{
"role": "assistant",
"message": "Hi, how can I help you?"
},
{
"role": "user",
"message": "I want to ..."
}
]
There was a problem hiding this comment.
Yepp, I think I'm splitting it into system and conversation right now. I can make that change.
…y to using a dictionary with user/assistant format
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 18
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
arklex/resources/workers/output_process/output_process_worker.py
Outdated
Show resolved
Hide resolved
arklex/resources/workers/output_process/output_process_worker.py
Outdated
Show resolved
Hide resolved
arklex/resources/workers/output_process/output_process_worker.py
Outdated
Show resolved
Hide resolved
arklex/resources/workers/output_process/output_process_worker.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| system_prompt: str = system_template.invoke( | ||
| {"sys_instruct": state.sys_instruct} | ||
| ).text | ||
| log_context.info(f"System prompt: {system_prompt}") |
There was a problem hiding this comment.
Debug print statements need removal before merge
Multiple log_context.info() statements logging system prompts and user prompts were added for testing purposes. Per the PR discussion, these were explicitly marked "To be removed after testing" but are still present in the code. These include logging in utils.py lines 61, 119, 176, message_worker.py line 84, and output_process_worker.py lines 115-120.
Additional Locations (2)
| system_prompt: str = system_template.invoke( | ||
| {"sys_instruct": state.sys_instruct} | ||
| ).text | ||
| log_context.info(f"System prompt: {system_prompt}") |
There was a problem hiding this comment.
Debug logging statements left in code before merge
The author explicitly noted "ToDo: Print lines need to be removed after testing before merging to main" and multiple "To be removed after testing" comments in the PR discussion. There are log_context.info() statements that log system prompts and user prompts across multiple files (utils.py, message_worker.py, output_process_worker.py, rag_message_worker.py). System prompts may contain sensitive instructions and should not be logged in production. These debug statements need to be removed before merging.
Additional Locations (2)
2 participants
Summary
Updating prompt templates to provide system prompt and conversation history separately.
Description
We're switching from providing one prompt that contains the system prompt and conversation history to providing these separately to the GenAI providers
Tests
Reviewers
@arklexai/agent-leads
Note
Modernizes model interactions to use structured chat messages instead of concatenated strings.
_format_messages()inModelServiceand updatesget_response/get_response_with_structured_outputto acceptsystem_promptandconversation_history; streaming paths now passmessagesConvoMessage.historyfromstrtolist[dict[str,str]]; updates all workers (MessageWorker,OutputProcessWorker,SearchWorker,MilvusRAGWorker,RagMsgWorker) and RAG retrievers to format history strings as needed (often including current user turn)*_system) and moves conversation/context into user prompts; introducesget_prompt_templates()helperutils/prompts.py, removes inline assistant markers, and aligns EN/CN variantsWritten by Cursor Bugbot for commit 4b6d416. This will update automatically on new commits. Configure here.