Skip to content

feat: switch to openassistant/core for AI module#11

Merged
lixun910 merged 22 commits intomainfrom
xli-try-openassist
Feb 25, 2025
Merged

feat: switch to openassistant/core for AI module#11
lixun910 merged 22 commits intomainfrom
xli-try-openassist

Conversation

@lixun910
Copy link
Collaborator

WIP:

  • try openassistant/core to power AI assistant in SQLRooms
  • use local context to avoid sending query results back to LLMs
  • use streaming response
  • add more provider and models

sqlroom-1

@lixun910 lixun910 marked this pull request as draft February 21, 2025 07:50
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

LGTM

@ibgreen
Copy link
Collaborator

ibgreen commented Feb 21, 2025

@kargeor

Copy link
Collaborator

@ilyabo ilyabo left a comment

Choose a reason for hiding this comment

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

thanks Xun, great to see this coming together!
I left a few comments

structuredOutputs: true,
});
},
getApiKey: () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we don't need createModel anymore. So we should redesign this interface. I think we need to pass:

  1. A list of models we allow users to choose from which can be from multiple providers
  2. API keys for all providers, maybe like this: { openai: 'ek....', anthropic: '....', }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we can definitely do that 👍

@ilyabo
Copy link
Collaborator

ilyabo commented Feb 21, 2025

I noticed something: I tried to ask a few questions, at some point it did run a few queries but didn't show any result. Why could that be?

image
{
  "id": "iz5ao0ow23y44ttoe7345ybf",
  "prompt": "make the timestamps and coordinates more human readable ",
  "toolResults": [],
  "toolCalls": [
    {
      "type": "tool-call",
      "toolCallId": "call_M6sVTqsfxWnDbZQlRODAjMlH",
      "toolName": "query",
      "args": {
        "type": "query",
        "sqlQuery": "SELECT DateTime, Latitude, Longitude, Magnitude FROM earthquakes ORDER BY Magnitude DESC LIMIT 5;",
        "reasoning": "To convert the Unix timestamps to a human-readable format, I will use the appropriate SQL function to format the DateTime."
      }
    },
    {
      "type": "tool-call",
      "toolCallId": "call_lK6oJpgLVbGsB0fBBQ5vjRnG",
      "toolName": "query",
      "args": {
        "type": "query",
        "sqlQuery": "SELECT TO_TIMESTAMP(DateTime / 1000) AS HumanReadableDateTime, Latitude, Longitude, Magnitude FROM earthquakes ORDER BY Magnitude DESC LIMIT 5;",
        "reasoning": "To convert the Unix timestamps to a human-readable format, I will use the TO_TIMESTAMP function."
      }
    },
    {
      "type": "tool-call",
      "toolCallId": "call_C4CM0C1BX8g18G5qVipvmaVt",
      "toolName": "query",
      "args": {
        "type": "query",
        "sqlQuery": "SELECT DateTime, Latitude, Longitude, Magnitude FROM earthquakes ORDER BY Magnitude DESC LIMIT 5;",
        "reasoning": "To get the original data for comparison."
      }
    }
  ],
  "analysis": "",
  "isCompleted": true
}

@ibgreen
Copy link
Collaborator

ibgreen commented Feb 21, 2025

I noticed something: I tried to ask a few questions, at some point it did run a few queries but didn't show any result. Why could that be?

Yes this is big impact area of improvement. Better error handling, explaining when things failed, how they failed, simply concluding that the failed. Anything is better than silent failures.

@lixun910
Copy link
Collaborator Author

Wow, Thanks for your quick response! 😁 Great comments! Yes, I am still working on this draft. There is something I need to change in openassistant/core package as well. Will address the comments and keep you updated.

@lixun910
Copy link
Collaborator Author

@ilyabo

sqlroom-2

@lixun910 lixun910 marked this pull request as ready for review February 22, 2025 05:58
@lixun910 lixun910 changed the title [Draft] try openassistant/core [Feat] switch to openassistant/core for AI module Feb 22, 2025
@lixun910
Copy link
Collaborator Author

Screenshot 2025-02-21 at 10 54 38 PM Screenshot 2025-02-21 at 10 54 51 PM

@ilyabo
Copy link
Collaborator

ilyabo commented Feb 22, 2025

I ❤️ the ability to view the "Query result", wanted to do the same thing.
Maybe it should be opening up in a modal, like the one when clicking on a table in the sidebar. For example, @ibgreen is actually using the AI chat in a sidebar panel, so there's little horizontal space.

image

@lixun910
Copy link
Collaborator Author

@ilyabo query table modal is used. Please help to re-review :)

I will create another branch to add more model providers

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we shouldn't reimplement QueryDataTable and DataTableModal, could we try to reuse them? If any changes are necessary, let's make the changes. They currently don't accept arrowTable, but I think we can just pass the query, this way we don't need to keep all the past query results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree. I am not confident to modify the DataTableModal (to break any other parts :). But I can create another PR to get this done after getting more familiar with the code.

@ilyabo
Copy link
Collaborator

ilyabo commented Feb 25, 2025

@lixun910 we should rename the PR to feat: switch to openassistant/core for AI module for the CI to pass. It uses conventional commits which are then grouped and included in the changelogs.

@ilyabo ilyabo changed the title [Feat] switch to openassistant/core for AI module feat: switch to openassistant/core for AI module Feb 25, 2025
@lixun910 lixun910 merged commit 5072e1f into main Feb 25, 2025
1 of 2 checks passed
@lixun910 lixun910 deleted the xli-try-openassist branch February 25, 2025 21:34
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

Comments