Skip to content

Refactor output of the query method in the Conversation class#297

Open
fcarli wants to merge 7 commits into
dev-model-additions-gem-litefrom
dev-query-output
Open

Refactor output of the query method in the Conversation class#297
fcarli wants to merge 7 commits into
dev-model-additions-gem-litefrom
dev-query-output

Conversation

@fcarli

@fcarli fcarli commented May 13, 2025

Copy link
Copy Markdown
Contributor

Summary

Refactor the output of the Conversation class to improve clarity and consistency.

Description

This PR introduces a refactor of the query method in the Conversation class. Previously, the method returned a tuple of position-based values, (response, token_usage, error_flag), which was a bit unclear.

To address this, I introduced a new QueryResult dataclass to encapsulate the output in a structured and self-documenting format:

@dataclass
class QueryResult:
    """Encapsulates the result of a conversation query."""
    query: str
    response: str | None = None
    correction: str | None = None
    token_usage: int | None = None
    error: bool | None = None

This change is conceptually straightforward but required a refactor across the codebase to accommodate the new return type. All existing tests have been updated accordingly, and the test suite passes successfully.

This refactor improves readability and future maintainability and reduces the risk of misinterpreting outputs in downstream logic.

@fcarli fcarli requested a review from slobentanzer May 13, 2025 17:10
@slobentanzer

Copy link
Copy Markdown
Contributor

Hi @fcarli,
thanks for the PR. It makes sense, obviously, but I am wondering whether we consider downstream effects on current dependent packages yet.

  • BioChatter Light: depends on return structure in the chat applications. Would need to be adjusted to benefit from BioChatter updates.
  • BioChatter Server: encapsulates chat responses into REST. Would need to be adjusted to benefit from BioChatter updates.
  • BioChatter Next: connects to BioChatter server, so probably reasonably isolated (given we update BioChatter Server).
  • Anything else that is dependent?

The most robust approach would be to synchronise a release. I fear that changing it in BioChatter and not simultaneously in the dependent packages will introduce some avoidable technical debt. How do you think it should be approached?

I suppose the new and old interfaces cannot be made compatible for legacy support without changing the downstream implementation.

@fcarli

fcarli commented May 19, 2025

Copy link
Copy Markdown
Contributor Author

Hi @fcarli, thanks for the PR. It makes sense, obviously, but I am wondering whether we consider downstream effects on current dependent packages yet.

  • BioChatter Light: depends on return structure in the chat applications. Would need to be adjusted to benefit from BioChatter updates.
  • BioChatter Server: encapsulates chat responses into REST. Would need to be adjusted to benefit from BioChatter updates.
  • BioChatter Next: connects to BioChatter server, so probably reasonably isolated (given we update BioChatter Server).
  • Anything else that is dependent?

The most robust approach would be to synchronise a release. I fear that changing it in BioChatter and not simultaneously in the dependent packages will introduce some avoidable technical debt. How do you think it should be approached?

I suppose the new and old interfaces cannot be made compatible for legacy support without changing the downstream implementation.

@slobentanzer I actually had the idea of introducing this refactor after looking at how the conversation query method was called in the BioChatter light repository. Since I'm currently working on implementing Gemini as the new model, we could try to include this change in the upcoming PR as well. As for BioChatter Server, I can look into it!

Perhaps the best strategy will be to push the tool usage PR on BioChatter first, then integrate this one afterward, synchronizing it with BioChatter-light and BioChatter-server. What is your opinion on this?

@slobentanzer

Copy link
Copy Markdown
Contributor

@fcarli that's what I would have suggested, too. :)

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