feat: switch to openassistant/core for AI module#11
Conversation
ilyabo
left a comment
There was a problem hiding this comment.
thanks Xun, great to see this coming together!
I left a few comments
| structuredOutputs: true, | ||
| }); | ||
| }, | ||
| getApiKey: () => { |
There was a problem hiding this comment.
Looks like we don't need createModel anymore. So we should redesign this interface. I think we need to pass:
- A list of models we allow users to choose from which can be from multiple providers
- API keys for all providers, maybe like this:
{ openai: 'ek....', anthropic: '....', }
There was a problem hiding this comment.
Yes, we can definitely do that 👍
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. |
|
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. |
|
I ❤️ the ability to view the "Query result", wanted to do the same thing.
|
|
@ilyabo query table modal is used. Please help to re-review :) I will create another branch to add more model providers |
packages/ai/src/QueryResult.tsx
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@lixun910 we should rename the PR to |
05ccc15 to
abb5746
Compare
abb5746 to
f42c830
Compare





WIP: