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

feat(agents-api): Make chat route tests pass #454

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Aug 12, 2024

Signed-off-by: Diwank Tomer [email protected]


🚀 This description was created by Ellipsis for commit 6279937

Summary:

This PR adds an /embed route for text embedding, refactors ChatMLMessage to InputChatMLMessage, updates chat logic, and includes tests and SDK updates.

Key points:

  • Add /embed route for text embedding in agents-api/routers/docs/embed.py.
  • Refactor ChatMLMessage to InputChatMLMessage across the codebase.
  • Update chat logic in agents-api/sessions.py.
  • Enhance template rendering with render_template_chatml.
  • Improve document search in agents-api/search_docs_hybrid.py.
  • Refactor chat route into get_messages and chat functions.
  • Add test for /embed route in agents-api/tests/test_docs_routes.py.
  • Add and update tests, including test_chat_routes.py.
  • Update TypeScript and Python SDKs for model changes.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 5f91ac3 in 1 minute and 45 seconds

More details
  • Looked at 1215 lines of code in 32 files
  • Skipped 2 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. agents-api/agents_api/autogen/Chat.py:90
  • Draft comment:
    The type of delta in ChatOutputChunk has been changed from ChatMLMessage to InputChatMLMessage. Ensure that this change is intended and that InputChatMLMessage provides all necessary fields that were previously used in ChatMLMessage.
  • Reason this comment was not posted:
    Confidence of 0% on close inspection, compared to threshold of 50%.
2. agents-api/agents_api/autogen/Entries.py:17
  • Draft comment:
    The role field in BaseEntry has been updated to include assistant instead of agent. Ensure that all references and logic in the codebase that depend on this field are updated accordingly to handle the new assistant role.
  • Reason this comment was not posted:
    Confidence of 0% on close inspection, compared to threshold of 50%.
3. agents-api/agents_api/autogen/Entries.py:122
  • Draft comment:
    The role field in InputChatMLMessage has been updated to include assistant instead of agent. Ensure that all references and logic in the codebase that depend on this field are updated accordingly to handle the new assistant role.
  • Reason this comment was not posted:
    Confidence of 0% on close inspection, compared to threshold of 50%.

Workflow ID: wflow_ehe5DBIjxRoRbwCf


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on dcc54c4 in 36 seconds

More details
  • Looked at 180 lines of code in 9 files
  • Skipped 2 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. typespec/chat/models.tsp:191
  • Draft comment:
    The logprobs field in BaseChatOutput, usage field in BaseChatResponse, and bytes field in BaseTokenLogProb should be updated to be optional to match the changes made in the TypeScript SDK.
logprobs?: LogProbResponse;
usage?: CompetionUsage;
bytes?: uint16[];
  • Reason this comment was not posted:
    Multiple suggestions found in comment body, which is not allowed by GitHub

Workflow ID: wflow_iGNmsuK0lpv0n58c


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 6279937 in 32 seconds

More details
  • Looked at 90 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_JR9jYfqLaqvjSAHE


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

):
(embed, _) = mocks

response = make_request(
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding an assertion to check the status code of the response to ensure the request was processed successfully. For example:

Suggested change
response = make_request(
assert response.status_code == 200

@creatorrr creatorrr changed the title wip: feat(agents-api): Make chat route tests pass feat(agents-api): Make chat route tests pass Aug 13, 2024
@whiterabbit1983 whiterabbit1983 merged commit 67db1fd into dev-tasks-disable-routes Aug 13, 2024
2 of 5 checks passed
@whiterabbit1983 whiterabbit1983 deleted the f/chat-tests branch August 13, 2024 05:43
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.

2 participants