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): Add embeddings to doc get/list response #578

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

HamadaSalhab
Copy link
Contributor

@HamadaSalhab HamadaSalhab commented Oct 3, 2024

Important

Add embeddings to document retrieval and listing responses in Docs.py, get_doc.py, list_docs.py, and models.tsp.

  • Behavior:
    • Add embeddings field to Doc model in Docs.py and models.tsp.
    • Include embeddings in the response of get_doc() in get_doc.py and list_docs() in list_docs.py.
  • Queries:
    • Update get_query in get_doc.py to retrieve embedding data.
    • Update get_query in list_docs.py to include embedding data.
  • Models:
    • Modify Doc model in models.tsp to include optional embeddings field with read visibility.

This description was created by Ellipsis for 70eebc4. It will automatically update as commits are pushed.

@HamadaSalhab HamadaSalhab merged commit 85e9929 into dev Oct 3, 2024
5 of 6 checks passed
@HamadaSalhab HamadaSalhab deleted the f/get-embedding-support branch October 3, 2024 14:21
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 70eebc4 in 22 seconds

More details
  • Looked at 86 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. agents-api/agents_api/models/docs/get_doc.py:40
  • Draft comment:
    Consider adding validation for the embeddings field to ensure it is of the expected type (list of floats or list of list of floats). This will prevent potential issues if the data does not match the expected type.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is about a change made in the diff, specifically the addition of 'embeddings'. Adding validation for data types can prevent runtime errors and ensure data integrity. However, the comment is speculative as it assumes potential issues without evidence of current problems.
    The comment might be unnecessary if the data source guarantees the type of 'embeddings', or if there are existing validations elsewhere in the codebase.
    Without evidence of existing validation or guarantees from the data source, the suggestion to add validation is reasonable to prevent future issues.
    The comment is relevant to the changes made and suggests a potential improvement in data validation, which is actionable and clear.
2. agents-api/agents_api/models/docs/list_docs.py:37
  • Draft comment:
    Consider adding validation for the embeddings field to ensure it is of the expected type (list of floats or list of list of floats). This will prevent potential issues if the data does not match the expected type.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_cryTiEcOnjlzfuyt


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

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.

1 participant