-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Postgresql Agent #3001
Postgresql Agent #3001
Conversation
@microsoft-github-policy-service agree |
Nice PR! @mdfahad999, @MihirDavada, @ChristianWeyer, @tyler-suard-parker, @Namec999, @tytung2020, @walker-null-byte, @nengisuls for awareness and it would be great if you could help testing and reviewing this PR! |
@yonitjio could you resolve the conflict in setup.py. Thanks! |
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
11616921 | Triggered | Generic High Entropy Secret | d02303a | notebook/agentchat_agentops.ipynb | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
What happened? Did I make a mistake? The gitguardian points to a file from a commit from another PR but why the name is main? I tried to sync from my fork but it seems it has no effect. |
Hi @yonitjio, sorry for the confusion. It is not caused by this PR and it is a false alarm. Please ignore it. |
Thanks for clarifying. |
|
||
**This will execute SQL SELECT query statement on PostgreSQL server.** | ||
|
||
The code blocks are executed or save in the order they are received. |
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 code blocks are executed or save in the order they are received. | |
The code blocks are executed or saved in the order they are received. |
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'll remove it instead since the sql will only be executed, never saved:
The code blocks are executed in the order they are received.
Is that alright?
skip or skip_openai, | ||
reason="dependency is not installed OR requested to skip", | ||
) | ||
def test_postgresql_agent(): |
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.
@yonitjio can you please check, why this test is skipping?
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 it's because the skip_openai
variable are set from contrib-test.yml
.
sys.path.append(os.path.join(os.path.dirname(__file__), "../..")) | ||
from conftest import skip_openai # noqa: E402 | ||
|
||
sys.path.append(os.path.join(os.path.dirname(__file__), "..")) |
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.
this line too.
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.
Will do, but I'm going to need to change the following line too:
from from test_assistant_agent import KEY_LOC, OAI_CONFIG_LIST # noqa: E402
to from agentchat.test_assistant_agent import KEY_LOC, OAI_CONFIG_LIST # noqa: E402
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.
@yonitjio thanks for the PR!
I was wondering if this design could also work for other sql databases as well, not just postgres. Have you considered using sqlalchemy as well?
I have, but I didn't think I can complete a PR using sqlchemy or similar library (someone suggested using pandasai in issue #28). I already used this method for my experiment using Autogen with Odoo which uses Postgresql, so I didn't really do much for this PR. I believe the more flexible way to do it is to have the these line as a callback: with pg.connect(self.dsn, row_factory=dict_row) as cnn:
with cnn.cursor() as cr:
cr.execute(code)
cres = cr.fetchall()
res_json = json.dumps(cres, default=str) Link:
This way the code becomes more generic and does not depend on any specific library, but one may argue this burdens the user with generic code. Of course, using connector library as you suggest is also feasible. Autogen's coding package is very flexible, this is just one example of it. |
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.
Thank you very much for the PR, @yonitjio ! I left a few comments, but I'm OK to see the sql support being added as a code language support in another PR.
from autogen.coding import CodeExecutor, CodeExtractor, MarkdownCodeExtractor, CodeBlock, CodeResult | ||
from autogen.runtime_logging import log_new_agent, logging_enabled | ||
|
||
class PostgreSqlQueryExecutor(CodeExecutor): |
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.
This class seems to be better put in the coding module.
def restart(self) -> None: | ||
return | ||
|
||
class PostgreSqlAgent(ConversableAgent): |
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.
If we go even further, this class is not very much needed once we extend the existing code executor to support sql
language. The cnn
could be passed into the executor.
retrieve_chat_pgvector.extend(["psycopg>=3.1.18"]) | ||
psycopg.extend(["psycopg>=3.1.18"]) | ||
|
||
retrieve_chat_pgvector.extend(psycopg) |
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.
Why would this PR change the setting of retrieve_chat_pgvector
? Is there an issue with current retrieve_chat_pgvector
?
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.
You can fix the code-formatting error with: pre-commit run --all-files
.
To run pre-commit
as part of git workflow, use pre-commit install
.
@thinkall I could not agree more with you on this. This PR should not be merged to I think we should continue the discussion in issue #28 for better implementation. |
Why are these changes needed?
This is an alternative approach to run SQL queries on PostgreSQL database without using function calling.
Mostly copy-pasted from coding package and user proxy agent.
Related issue number
Serve as an example for issue #28
Answer discussion #1558
Checks