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

Feature/allow backwards compat #1964

Merged
merged 2 commits into from
Feb 11, 2025
Merged

Conversation

emrgnt-cmplxty
Copy link
Contributor

@emrgnt-cmplxty emrgnt-cmplxty commented Feb 11, 2025

Important

Reintroduce and deprecate completion field in RAGResponse for backwards compatibility, updating rag() function accordingly.

  • Models:
    • In RAGResponse class, comment out generated_answer field and reintroduce completion field with deprecation notice in responses.py.
  • Services:
    • Update rag() function in retrieval_service.py to use completion field in RAGResponse instead of generated_answer.

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

@emrgnt-cmplxty emrgnt-cmplxty marked this pull request as ready for review February 11, 2025 07:37
@emrgnt-cmplxty emrgnt-cmplxty merged commit af648a5 into main Feb 11, 2025
2 checks passed
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 22adf3c in 1 minute and 2 seconds

More details
  • Looked at 52 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. py/core/main/services/retrieval_service.py:598
  • Draft comment:
    The RAGResponse is constructed with a 'generated_answer' field, but this field is now commented out in the model. This may cause a validation error. Use only the 'completion' field or update the model to accept both names for backwards compatibility.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. py/shared/api/models/retrieval/responses.py:109
  • Draft comment:
    The generated_answer field is commented out in favor of a new 'completion' field. Also, the JSON schema example still references 'generated_answer'. Update the schema example to reflect the new field for consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. py/core/main/services/retrieval_service.py:600
  • Draft comment:
    Adding the parameter 'completion' now in the RAGResponse constructor. Make sure that downstream clients and documentation match the new field name, as this replaces the previously named 'generated_answer'.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the author to ensure that downstream clients and documentation match the new field name. This is a request for confirmation and not a specific code suggestion or test request. It violates the rule against asking the author to ensure behavior is intended or to double-check things.
4. py/shared/api/models/retrieval/responses.py:107
  • Draft comment:
    The 'generated_answer' field and its alias property 'completion' were removed in favor of a sole 'completion' field marked as deprecated. This appears to reverse the original intent (which instructed to use 'generated_answer' instead). Also, the JSON schema example still shows 'generated_answer'. Please clarify which field should be canonical and update both the deprecation notice and schema example accordingly.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_i9DrFSuywzACKRGk


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

@emrgnt-cmplxty emrgnt-cmplxty deleted the feature/allow-backwards-compat branch February 12, 2025 20:03
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