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

Mismatch in the comment and the code for properties of CohereCallResponseChunk class #293

Closed
tvj15 opened this issue May 30, 2024 · 2 comments

Comments

@tvj15
Copy link
Contributor

tvj15 commented May 30, 2024

In the code below, the comment says that the response property returns the response for the text-generation event type. But the code checks for Stream End (StreamedChatResponse_StreamEnd) event type.

@property
    def response(self) -> Optional[NonStreamedChatResponse]:
        """Returns the response for text-generation event type else None."""
        if isinstance(self.chunk, StreamedChatResponse_StreamEnd):
            return self.chunk.response
        return None

The similar thing goes here:

  @property
    def documents(self) -> Optional[list[ChatDocument]]:
        """Returns the documents for citation-generation event type else None."""
        if isinstance(self.chunk, StreamedChatResponse_SearchResults):
            return self.chunk.documents
        return None
@tvj15 tvj15 changed the title Mismatch in the comment and the code for response property of CohereCallResponseChunk class Mismatch in the comment and the code for properties of CohereCallResponseChunk class May 30, 2024
@willbakst willbakst assigned willbakst and unassigned willbakst May 31, 2024
@willbakst
Copy link
Contributor

So I believe the first one is actually correct. The StreamedChatResponse_StreamEnd chunk type has a response property that returns the NonStreamedChatResponse instance, which has the same type as the standard text-generation event (and the full response for the entire text-generation stream). Maybe this could be more clear, something like "Returns the full response for the stream-end event type else None."?

The second one should say "search-results event type" I believe, good catch!

Want to open a PR with the fix?

@tvj15
Copy link
Contributor Author

tvj15 commented Jun 3, 2024

So I believe the first one is actually correct. The StreamedChatResponse_StreamEnd chunk type has a response property that returns the NonStreamedChatResponse instance, which has the same type as the standard text-generation event (and the full response for the entire text-generation stream). Maybe this could be more clear, something like "Returns the full response for the stream-end event type else None."?

The second one should say "search-results event type" I believe, good catch!

Want to open a PR with the fix?

Sure, on it!

tvj15 added a commit to tvj15/mirascope that referenced this issue Jun 6, 2024
brenkao added a commit that referenced this issue Jun 6, 2024
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

No branches or pull requests

2 participants