-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Improve RetrieveChat #6
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6 +/- ##
==========================================
+ Coverage 34.22% 35.16% +0.94%
==========================================
Files 17 17
Lines 1911 1965 +54
Branches 416 465 +49
==========================================
+ Hits 654 691 +37
- Misses 1207 1223 +16
- Partials 50 51 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This PR decreases coverage. Can we avoid that? |
I see |
Some tests are skipped because chromadb is not installed in [test]. We can add it. |
Updated to 0.1.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw a chunk of code that was commented out in generate_init_message.
Also consider making _generate_retrieve_user_reply more readable.
@@ -230,6 +230,11 @@ def _generate_retrieve_user_reply( | |||
sender: Optional[Agent] = None, | |||
config: Optional[Any] = None, | |||
) -> Tuple[bool, Union[str, Dict, None]]: | |||
"""In this function, we will update the context and reset the conversation based on different conditions. | |||
We'll update the context and reset the conversation if no_update_context is False and either of the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be easier to think of "update_context is True" than "no_update_context is False".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. once suggestion that you could consider is to rename the rerieve_user_proxy_agent.py as RAG_user_proxy_agent.py.
* Upsert in batch * Improve update context, support customized answer prefix * Update tests * Update intermediate answer * Fix duplicate intermediate answer, add example 6 to notebook * Add notebook results * Works better without intermediate answers in the context * Bump version to 0.1.2 * Remove commented code and add descriptions to _generate_retrieve_user_reply --------- Co-authored-by: Qingyun Wu <[email protected]>
* intial commit for aws-bedrock * format * converse setup for model req-response * Renamed to bedrock.py, updated parameter parsing, system message extraction, client class incorporation * Established Bedrock class based on @astroalek and @ChristianT's code, added ability to disable system prompt separation * Image parsing and removing access credential checks * Added tests, added additional parameter support * Amazon Bedrock documentation * Moved client parameters to init, align parameter names with Anthropic, spelling fix, remove unnecessary imports, use base and additional parameters, update documentation * Tidy up comments * Minor typo fix * Correct comment re aws_region --------- Co-authored-by: Hk669 <[email protected]> Co-authored-by: HRUSHIKESH DOKALA <[email protected]>
add envvars in all projects
* namespace fixes + remove skills definitios from Actors project * add waf context to actors * deploy to Azure WIP * add bicep for gh-flow and cosmos * azure deploy fixes * azure deploy WIP
* Renamed TeamOne to MagenticOne * Updated uv.lock * Fixed workflows.
Why are these changes needed?
Update Context
logic, support customizing trigger wordscustomized_answer_prefix
. Ifcustomized_answer_prefix
is given and it is not in the answer,Update Context
will also be triggered.no_update_context
to stopUpdate Context
, useful for ablation experiments.RetrieveChat
notebook.Related issue number
Checks