search

RamiAwar / dataline

Chat with your data - AI data analysis and visualization on CSV, Postgres, MySQL, Snowflake, SQLite...
https://dataline.app
GNU General Public License v3.0
130 stars 6 forks source link

fix: Attempting to patch the missing tool message bug #206

Closed RamiAwar closed 2 weeks ago

RamiAwar commented 3 weeks ago

Closes #204

RamiAwar commented 3 weeks ago

Reason to separate them was to ensure that all tool messages come first (from across calls to tools) and all other messages come later.

but if you said that this didn't fix the bug, then we can revert this cause it's a useless change and only adds confusion and doesn't fix the problem

anthony2261 commented 3 weeks ago

Reason to separate them was to ensure that all tool messages come first (from across calls to tools) and all other messages come later.

I meant why seperate the content into ToolMessage and AIMessage, instead of just having ToolMessage. In QuerySQLDataBaseTool.get_response, we have a tool message with content like

content="Query executed successfully - here is the returned data description. "
"I cannot view the data for security reasons but the user should be able to see the results!"
f"{data_description}"

And then an AIMessage with content like:

"Now that I have the data, I can analyze it and consider regenerating the query."
...

Why not add the content of the AIMessage into the ToolMessage and get rid of the AIMessage altogether?

I don't see these messages as just sending the AI anything. As in how you structure the messages influences the LLM responses. For example if I send tool output (sql) in the AIMessage, it will start sending back to the user (sql) in the message because this is what we 'taught' it from convo history.

I'm also not sure if there will be other downsides, so I avoid it. Doing it will introduce some tiny bugs/unexpected outputs in some places (0.99 probability), and we have no way of really knowing the consequences. Safer to keep it this way as the langchain library recommends too, other devs will understand this better than stuffing everything into tool messages.

Back to the point above, I think we shouldn't combine the tool messages in the beginning if you say the bug is still there. We can return it to how it was previously, guess the bug is from somewhere else.

anthony2261 commented 2 weeks ago

@RamiAwar check this: https://github.com/RamiAwar/dataline/commit/5a8058da66ae72ba8f5bf37596b5ea18c335887f This fixed things for me without having to create the split between tool_messages and other messages like we do in this branch. Also, this makes it such that each tool returns one ToolMessage and nothing else