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 embed_instruction argument to embeddings & document creation endpoints #715

Merged
merged 9 commits into from
Oct 22, 2024

Conversation

HamadaSalhab
Copy link
Contributor

@HamadaSalhab HamadaSalhab commented Oct 19, 2024

Important

Adds embed_instruction argument to document creation and embedding endpoints, updating models and OpenAPI spec accordingly.

  • Behavior:
    • Adds embed_instruction argument to CreateDocRequest and EmbedQueryRequest in Docs.py.
    • Updates create_doc() in create_doc.py to handle embed_instruction by removing it from doc_data.
    • Modifies run_embed_docs_task() in create_doc.py to pass embed_instruction.
    • Updates embed() in embed.py to prepend embed_instruction to text_to_embed.
  • OpenAPI Specification:
    • Adds embed_instruction to CreateDocRequest and EmbedQueryRequest models in models.tsp.
    • Updates openapi-0.4.0.yaml and openapi-1.0.0.yaml to include embed_instruction in required properties for CreateDocRequest and EmbedQueryRequest.

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

@HamadaSalhab HamadaSalhab marked this pull request as draft October 19, 2024 11:17
Copy link

gitguardian bot commented Oct 19, 2024

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
14169429 Triggered Generic High Entropy Secret ea49775 agents-api/notebooks/main-3-Copy1.ipynb View secret
14169430 Triggered Generic High Entropy Secret ea49775 agents-api/notebooks/main-3.ipynb View secret
14169430 Triggered Generic High Entropy Secret ea49775 agents-api/notebooks/main-3.ipynb View secret
14169429 Triggered Generic High Entropy Secret ea49775 agents-api/notebooks/main-3-Copy1.ipynb View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

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 a006be6 in 43 seconds

More details
  • Looked at 439 lines of code in 10 files
  • Skipped 3 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:2446
  • Draft comment:
    The embed_instruction field is marked as required here, but it is optional in the code. Consider making it optional in the OpenAPI spec to match the implementation.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment addresses a change made in the diff, specifically the addition of the 'embed_instruction' field as required. If the field is indeed optional in the code, this is a valid concern as it could lead to discrepancies between the API documentation and the actual API behavior. This could cause issues for developers relying on the spec.
    I might be missing the actual implementation details of the code, which are not visible in the diff. The comment assumes that the field is optional in the code, but without seeing the code, this is speculative.
    The comment is based on the assumption that the field is optional in the code, which is a reasonable assumption if the field is nullable and has a default value of null. This suggests that the field might not be required in practice, even if marked as required in the spec.
    The comment should be kept as it highlights a potential inconsistency between the OpenAPI spec and the code implementation, which could lead to issues if not addressed.

Workflow ID: wflow_sNpcedNNT88DZ2FS


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

@creatorrr
Copy link
Contributor

@HamadaSalhab You checked in the local jwt

@HamadaSalhab
Copy link
Contributor Author

@creatorrr They were already there, the changes are there due to reformatting. Btw, we should delete these notebooks. I don't see the point in having them here.

@HamadaSalhab HamadaSalhab marked this pull request as ready for review October 22, 2024 15:17
@HamadaSalhab HamadaSalhab self-assigned this Oct 22, 2024
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 1e22e30 in 55 seconds

More details
  • Looked at 453 lines of code in 10 files
  • Skipped 3 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:2446
  • Draft comment:
    The embed_instruction field is marked as required here, but in the implementation, it is optional. Consider making it optional in the OpenAPI spec as well.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is directly related to the changes made in the diff, where 'embed_instruction' was added as a required field. If the implementation indeed treats it as optional, the comment is valid and suggests a necessary change to align the spec with the implementation.
    I might be missing the actual implementation details that confirm whether 'embed_instruction' is optional or required. Without seeing the implementation, it's speculative to assume the comment is correct.
    The comment is based on the assumption that the implementation treats 'embed_instruction' as optional. If this assumption is correct, the comment is valid. However, without seeing the implementation, it's hard to confirm.
    Keep the comment as it addresses a potential discrepancy between the OpenAPI spec and the implementation regarding the 'embed_instruction' field.
2. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:2580
  • Draft comment:
    The embed_instruction field is marked as required here, but in the implementation, it is optional. Consider making it optional in the OpenAPI spec as well.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_PkWqqi1xSmnd8JvM


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.

Skipped PR review on 4290875 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.

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 b68ab1d in 19 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/agents_api/routers/docs/embed.py:25
  • Draft comment:
    Ensure that embed_instruction is intended to be prepended to each text in text_to_embed. If not, consider revising the logic to handle embed_instruction appropriately.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The current implementation of the embed function concatenates embed_instruction with each text in text_to_embed. This could lead to unexpected results if embed_instruction is not intended to be prepended to each text. It would be better to clarify this behavior or ensure that it aligns with the intended use case.

Workflow ID: wflow_gzIo6ZLywLyo7Pkn


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

@HamadaSalhab HamadaSalhab merged commit 7f7fce9 into dev Oct 22, 2024
11 of 14 checks passed
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.

4 participants