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

python driver psycopg3 #1793

Merged
merged 2 commits into from
Apr 30, 2024
Merged

python driver psycopg3 #1793

merged 2 commits into from
Apr 30, 2024

Conversation

mhmaguire
Copy link
Contributor

Updated python driver code to psycopg 3 for my own purposes and figured you all might like to do the same sometime.

@aked21
Copy link
Contributor

aked21 commented Apr 27, 2024

Hi @mhmaguire, can you specify what have been changed and/or added for your needs?
Thank you for contributing!
AK

@dehowef
Copy link
Member

dehowef commented Apr 29, 2024

Wow! Thank you so much this is great.

@mhmaguire
Copy link
Contributor Author

@aked21 I attempted to change as little as possible and swap psycopg2 for v3. That being said some changes were necessary as psycopg3 introduces a new type adaptation system, no longer has an ext module, and doesn't have a replacement implementation for execute_values:

  • drivers/python/age/age.py
    • Loader subclass to cast ag_type. AgeLoader decodes bytes and passes the string to parseAgeValue.
    • It's unclear if it is necessary to implement a Dumper as well?
    • Additionally we can rely on TypeInfo.fetch(typename) to fetch type details from pg.
    • cursor_factotry=ClientCursor in Age constructor so that we have access to .mogrify().
    • unsure why previous implementation did this:
      cypher = cypher[2:len(cypher)-1]
      For me this was mangling the produced cypher statements so replaced with cypher = cypher.strip()
  • drivers/python/networkx/lib.py
    • execute_values in networkx/lib.py replaced with executemany
  • Misc
    • updated all type hints
    • ensure tests pass
    • updated tests so parsed args actually get passed to Age constructor
    • ensure notebooks work

@dehowef
Copy link
Member

dehowef commented Apr 30, 2024

@mhmaguire I pulled down your changes to take a look, and the test files are complaining with the line:

AttributeError: 'TestAgeBasic' object has no attribute 'args'

Granted I just took a cursory glance, but I wanted to ask, do the tests work on your machine? Thanks for your time.

@dehowef
Copy link
Member

dehowef commented Apr 30, 2024

@mhmaguire scratch my previous comment-- User error on my part! I'll continue reviewing this when I find a moment

@dehowef
Copy link
Member

dehowef commented Apr 30, 2024

@mhmaguire scratch my previous comment-- User error on my part! I'll continue reviewing this when I find a moment

Lol okay actually sorry to walk back my comment again, but when I run the unittests via "python -m unittest -v test_age_.py" and python -m unittest -v test_networkx.py as stated in the documentation, they throw an attribute error as follows:

Traceback (most recent call last):
File "/home/user/age/drivers/python/test_age_py.py", line 36, in setUp
host=self.args.host,
^^^^^^^^^
AttributeError: 'TestAgeBasic' object has no attribute 'args'

The unit tests still run fine if I call them directly, but I think it'd be a relatively easy fix to get this error to disappear. Would you mind amending your commit so that they work with these unit test commands? Otherwise it looks great and I appreciate what you've done here. I think after that change, I'll give it another once over and probably good to go. @mhmaguire

@mhmaguire
Copy link
Contributor Author

@dehowef I set a default prefilled namespace so tests won't fail when bypassing argparse.

python test_age_py.py would have a similar effect...

@dehowef
Copy link
Member

dehowef commented Apr 30, 2024

@dehowef I set a default prefilled namespace so tests won't fail when bypassing argparse.

python test_age_py.py would have a similar effect...

@mhmaguire Yeah, that way of testing was written by the original author of the python driver, been thinking of updating/removing that section of the documentation. I just looked over everything, and it looks great! We usually want to bring all updates to the main branch down to previous versions, from PG16 down til PG13. If you want to create the PRs for those branches as well, feel free to, otherwise I might just go ahead and do that within the next few days!! Thank you so much for your contribution 🙂

@dehowef dehowef merged commit db0a442 into apache:master Apr 30, 2024
7 checks passed
jrgemignani pushed a commit to jrgemignani/age that referenced this pull request May 2, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.
dehowef pushed a commit that referenced this pull request May 2, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Co-authored-by: Matt <[email protected]>
jrgemignani pushed a commit to jrgemignani/age that referenced this pull request May 3, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Resolved -

Conflicts:
	drivers/python/requirements.txt
jrgemignani pushed a commit to jrgemignani/age that referenced this pull request May 3, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Resolved -

Conflicts:
	drivers/python/requirements.txt
jrgemignani pushed a commit to jrgemignani/age that referenced this pull request May 3, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Resolved -

Conflicts:
	drivers/python/requirements.txt
jrgemignani pushed a commit to jrgemignani/age that referenced this pull request May 3, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Resolved -

Conflicts:
	drivers/python/requirements.txt
dehowef pushed a commit that referenced this pull request May 6, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Resolved -

Conflicts:
	drivers/python/requirements.txt

Co-authored-by: Matt <[email protected]>
dehowef pushed a commit that referenced this pull request May 6, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Resolved -

Conflicts:
	drivers/python/requirements.txt

Co-authored-by: Matt <[email protected]>
dehowef pushed a commit that referenced this pull request May 6, 2024
* update for psycopg3

* set a default argparse namespace  in case tests are run in such a way that argparse is bypassed.

Resolved -

Conflicts:
	drivers/python/requirements.txt

Co-authored-by: Matt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants