-
Notifications
You must be signed in to change notification settings - Fork 45
Adding Teradata VectorStore Tools #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
Hi @remi-td , Should I continue the modification of EVS, or did you have new plan? I'm converting EVS from using teradatagenai to a pure SQL approach. However, I've encountered a problem: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the teradataml.create_context() approach here won't work in case of a multi-user setup... When we tun the MCP server with a communication proxy (eg. AUTH_MODE=basic as explained here https://github.com/Teradata/teradata-mcp-server/blob/main/docs/server_guide/SECURITY.md), the conn_url/conn_username are the service user's (proxy) and not the end-user.
You need to reuse the TDConn() object to get the connection (with the appropriate proxy settings) to impersonate the end-user and avoid re-creating a connection (and context) every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that you need the sysdba ie. admin user name here, not the dbc... For a small test instance, we would use the dbc user, but in a production scenario, these would be two different database users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified dbc_user and dbc_password to sysdba_user and sysdba_password
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this means one PEM file for everyone, shared at the server level? With every MCP server user getting the same access level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we followed the similar approach for now, as it was present for existing tool for evs.
remi-td
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will need to figure out the connection mechanism so this also works in multi-user.
We also need a doc and an explanation on the above current limitation
* Adding VectorStore tools * Adding testcases * Making teradata_vectorstore tools optional * Updated testcases for Teradata VectorStore tools * Updated vs tool names with tdvs_ prefix and updated tests
Tests ran successfully for newly added tools.