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

python3.7 support #437

Merged
merged 17 commits into from
Aug 13, 2018
Merged

python3.7 support #437

merged 17 commits into from
Aug 13, 2018

Conversation

smagafurov
Copy link
Contributor

No description provided.

@smagafurov
Copy link
Contributor Author

Problem with python3.7
pypa/setuptools#1257

@codecov
Copy link

codecov bot commented Feb 14, 2018

Codecov Report

Merging #437 into master will increase coverage by 0.02%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
+ Coverage   93.57%   93.59%   +0.02%     
==========================================
  Files          25       25              
  Lines        3672     3684      +12     
  Branches      194      195       +1     
==========================================
+ Hits         3436     3448      +12     
  Misses        198      198              
  Partials       38       38
Impacted Files Coverage Δ
tests/test_pool.py 99.75% <100%> (ø) ⬆️
aiopg/cursor.py 98.76% <100%> (ø) ⬆️
aiopg/sa/result.py 91.08% <100%> (ø) ⬆️
aiopg/utils.py 84.07% <90.9%> (+0.96%) ⬆️
aiopg/sa/engine.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b670ea...b01c4c2. Read the comment docs.

@smagafurov
Copy link
Contributor Author

@asvetlov: fixes for python3.7 done

@vir-mir
Copy link
Member

vir-mir commented Feb 14, 2018

@smagafurov you lowered the coverage, could you raise it or leave it as before

@smagafurov
Copy link
Contributor Author

@vir-mir: Sorry but I can't understand how to do this.
See my changes.
May you help me to do this?

@vir-mir
Copy link
Member

vir-mir commented Feb 15, 2018

I'll try to look at this week

@smagafurov
Copy link
Contributor Author

@vir-mir: now code coverage is ok

!!! but there is some back incompatibility in python 3.7

This code works in python 3.6

async for value in conn.execute(sql):

But in python 3.7 it throws:

TypeError: 'async for' received an object from __aiter__ that does not implement __anext__: generator

I fix this for python 3.7 as:

async for value in await conn.execute(sql):

See my changes in tests/pep492/test_async_await.py

May be this solution is wrong and we must do something to support python3.7 without this incompatibility.

Copy link
Member

@vir-mir vir-mir left a comment

Choose a reason for hiding this comment

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

@asvetlov @jettify how do you think?

@@ -275,7 +278,10 @@
result = []
async with aiopg.sa.create_engine(loop=loop, **pg_params) as engine:
async with engine.acquire() as conn:
async for value in conn.execute(sql):
rows = conn.execute(sql)
if PY_37:
Copy link
Member

Choose a reason for hiding this comment

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

This is a bad decision. We are here solving the consequences and not the cause. you need to rethink it, you probably need to do this in conn.execute

Copy link

Choose a reason for hiding this comment

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

Can anyone else refactor this hack so we can move forward? I don't know what you mean by "doing this in conn.execute", @vir-mir.

Copy link

@JustinTArthur JustinTArthur Jul 19, 2018

Choose a reason for hiding this comment

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

This is a fun one. The examples and documentation show that aiopg.sa.SAConnection.execute is both await-able and can be asynchronously iterated BUT the current implementation should only work by awaiting result before iterating. It just happens to be working without awaiting the result in CPython < 3.7 because it thinks we were supplying an old-style async iterable and that it should await the iterable first like it did pre-3.5.2.

Now that CPython doesn't revert to awaiting when __aiter__ returns an awaitable, we have a couple choices:

  • Change the documentation, examples, tests to emphasize that execute is not async-for-able but its result is, like async for row in await conn.execute(tbl.select())

or

  • Leave docs, tests, examples as-is, but make our _SAConnectionContextManagers behave like they should, with their __aiter__ providing something other than a coroutine. This could be the _SAConnectionContextManager instance itself, with some __anext__ implemented, or some other wrapper around the coroutine the manager encapsulates.
    • This might be the best option given folks have probably already been using for row in conn.execute() in their code.

Choose a reason for hiding this comment

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

@smagafurov I've created a possible fix PR to your PR in smagafurov#1.

@mikhail-yarosh
Copy link

any update on this?

@asvetlov
Copy link
Member

Please postpone the merge.
I need to put my hands on the PR.

@@ -106,7 +106,8 @@ class Connection:

def __init__(self, dsn, loop, timeout, waiter, echo, **kwargs):
self._loop = loop
self._conn = psycopg2.connect(dsn, async=True, **kwargs)
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Gr1N
Copy link
Contributor

Gr1N commented Jul 6, 2018

Hi, any updates and estimates on this? Thanks

The behavior of iteration has changed to where a StopIteration exception
should not be raised inside a generator except by something unexpected.
@JustinTArthur
Copy link

@smagafurov I have two pull requests pending against your PR's branch for fixing some of the remaining issues.

smagafurov and others added 3 commits July 20, 2018 23:07
Fix for RuntimeError in Python 3.7+ from unexpected StopIteration raises
Await result before first async iteration in 3.5.2+ in SQL alchemy execute
Resolves Python 3.4 flake8 continuous integration failure.
@JustinTArthur
Copy link

This last one should address the failing CI checks for 3.4.

except StopAsyncIteration:
self._obj.close()
self._obj = None
raise

Choose a reason for hiding this comment

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

Ideally, we would just make __aiter__ an async generator so we don't have to supply this __anext__ proxying code, but this would break for the small handful of CPython releases >=3.5.2 and <3.6

@@ -9,8 +9,10 @@ python:
- "3.4"
- "3.5"
- "3.6"
- "nightly"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want to replace "nightly" -> "3.7" but there is issue about Travis and python 3.7: travis-ci/travis-ci#9815

Copy link
Member

Choose a reason for hiding this comment

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

You can add a job with specific settings for that

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind. I did this.

Choose a reason for hiding this comment

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

@webknjaz nice. Did you mean to leave in the nightly build? Not opposed to it either way.

Copy link
Member

Choose a reason for hiding this comment

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

Nightly points to 3.8-dev now.

@JustinTArthur
Copy link

@vir-mir and @asvetlov this should be ready for another pass if you get a moment.

@popravich
Copy link
Member

LGTM, but I'd rather wait for @asvetlov to share his thoughts on this

@txomon
Copy link

txomon commented Aug 4, 2018

I have run our battery of integration tests against this branch and has worked good. I see there are a couple of comments open, do they require any further action? We need into 3.7 in my project and right now this is the one dependency keeping us in 3.6, is there anything I can do to push this forward?

@vir-mir
Copy link
Member

vir-mir commented Aug 10, 2018

@asvetlov @popravich @jettify if there are no objections, I will wait a few days and salt it.

@ibnbay00
Copy link

Please... merge it.. I can't connect using latest psycopg2 :(

@vir-mir vir-mir merged commit 89c95e3 into aio-libs:master Aug 13, 2018
vir-mir pushed a commit to vir-mir/aiopg that referenced this pull request Jan 28, 2019
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.