Skip to content

Conversation

@cpsievert
Copy link
Contributor

@cpsievert cpsievert commented Nov 26, 2025

This PR does the following:

  1. Fixes a major issue introduced in feat(pkg-py): Add new QueryChat() API; hard deprecate old API #101 by dropping QueryChat.df(), QueryChat.sql(), etc. in favor of .server() now returning those values. I don't think there's a way we can support that API and also prevent one session from effecting another. This doesn't, however, mean we need change the Express API (in that case, the QueryChatExpress() instance is session-specific). So, in other words, the Core API now looks like this:
from seaborn import load_dataset
from shiny import App, render, ui
from querychat import QueryChat

titanic = load_dataset("titanic")

# 1. Provide data source to QueryChat
qc = QueryChat(titanic, "titanic")

app_ui = ui.page_sidebar(
    # 2. Create sidebar chat control
    qc.sidebar(),
    ui.card(
        ui.card_header(ui.output_text("title")),
        ui.output_data_frame("data_table"),
        fill=True,
    ),
    fillable=True
)


def server(input, output, session):
    # 3. Add server logic (to get reactive data frame and title)
    qc_vals = qc.server()

    # 4. Use the filtered/sorted data frame reactively
    @render.data_frame
    def data_table():
        return qc_vals.df()

    @render.text
    def title():
        return qc_vals.title() or "Titanic Dataset"


app = App(app_ui, server)
  1. As a consequence of 1, it no longer makes sense for QueryChat to have a client attribute (since you can read the session-specific one via .server()).
  2. While here, I decided to add a system_prompt property for convenience of inspecting the system prompt (for either class -- if a session-specific one is desired for some reason, it can be obtained via the client server value)
  3. Added a types modules where the return value for .server() can be imported, as well as the DataSource types. Relatedly, querychat.datasource was removed (import from querychat.types instead)

This comment was marked as resolved.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@cpsievert cpsievert merged commit 7c93d09 into main Nov 26, 2025
6 checks passed
@cpsievert cpsievert deleted the fix/session-scope branch November 26, 2025 17:05
@gadenbuie
Copy link
Contributor

Couldn't we attach these values in session.user_data and have the methods find the session-specific value? It's so much cleaner to use the methods on a single object that I think it'd be worth it

@cpsievert
Copy link
Contributor Author

cpsievert commented Nov 26, 2025

It's so much cleaner to use the methods on a single object that I think it'd be worth it

I think it's debate-able whether it's "cleaner". I do like that:

  1. It's less code/variables
  2. A bit more discoverable/documentable

However, I'm becoming more convinced that the "spooky" action of $server() being called for it's side effects makes things harder to reason about. That is, by having server return a value, it makes it clearer that those values are session-scoped, whereas the QueryChat() instance is app-scoped. I was actually discussing this point recently with Joe, and this is precisely the reason why modules are designed the way they are (instead of the "cleaner" single-object design)

@cpsievert
Copy link
Contributor Author

Also, if you feel super strongly about it, I think we could support both ways. But if we do, I want make sure we think through it carefully.

Since I want to un-block Veerle, let's not make that a requirement for now (it can be a opt-in feature add).


@dataclass
class ModServerResult:
class ServerValues:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how much this name matters (since users interact with instances of this class), but I'd rather something like QueryChatValues. ServerValue seems too generic.

More importantly, I think that returning a dataclass is an excellent compromise and I like this solution as a middle ground to the class instance methods approach we were looking at before.

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.

3 participants