Skip to content
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

Closed
wants to merge 4 commits into from
Closed

Postgresql Agent #3001

wants to merge 4 commits into from

Conversation

yonitjio
Copy link

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

@yonitjio
Copy link
Author

@microsoft-github-policy-service agree

@qingyun-wu qingyun-wu requested a review from thinkall June 22, 2024 18:26
@qingyun-wu
Copy link
Contributor

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!

@qingyun-wu
Copy link
Contributor

@yonitjio could you resolve the conflict in setup.py. Thanks!

@qingyun-wu qingyun-wu requested a review from sonichi June 22, 2024 18:29
Copy link

gitguardian bot commented Jun 23, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
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
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. 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


🦉 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.

@yonitjio
Copy link
Author

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.

@qingyun-wu
Copy link
Contributor

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.

@yonitjio
Copy link
Author

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Author

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():
Copy link
Collaborator

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?

Copy link
Author

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__), ".."))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line too.

Copy link
Author

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

Copy link
Contributor

@WaelKarkoub WaelKarkoub left a 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?

@yonitjio
Copy link
Author

yonitjio commented Jul 5, 2024

@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:

with pg.connect(self.dsn, row_factory=dict_row) as cnn:

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.

Copy link
Collaborator

@thinkall thinkall left a 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):
Copy link
Collaborator

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):
Copy link
Collaborator

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)
Copy link
Collaborator

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?

Copy link
Collaborator

@thinkall thinkall left a 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.

@yonitjio
Copy link
Author

yonitjio commented Jul 8, 2024

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.

@thinkall I could not agree more with you on this. This PR should not be merged to main branch as there are better ways for Autogen to support SQL with the coding package. I'm going to close this PR. I apologize for the inconvenience.

I think we should continue the discussion in issue #28 for better implementation.

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.

5 participants