scylladb / python-driver

ScyllaDB Python Driver, originally DataStax Python Driver for Apache Cassandra
https://python-driver.docs.scylladb.com
Apache License 2.0
70 stars 42 forks source link

Python 3.12 support [asyncore is deprecated] #257

Open jdavidagudelo opened 11 months ago

jdavidagudelo commented 11 months ago

The library can be installed in python 3.12 but any usage fails with this output:

./../../.cache/pypoetry/virtualenvs/project-py3.12/lib/python3.12/site-packages/cassandra/io/asyncorereactor.py:34: in <module>
 import asyncore
ModuleNotFoundError: No module named 'asyncore'

Is there any plans to replace asyncore and support python 3.12?

Lorak-mmk commented 11 months ago

For now, you can try to use GeventConnection / EventletConnection or LibevConnection (see connection_class parameter in Cluster.__init__). In order to make the default case work, we should make AsyncioConnection work and production ready and make it the default (we could maybe even remove asyncore one - I don't think we support any Python version that doesn't have asyncio). And this should be done in upstream, it's in no way Scylla specific.

Lorak-mmk commented 11 months ago

I see there is an issue in upstream: https://datastax-oss.atlassian.net/jira/software/c/projects/PYTHON/issues/PYTHON-1361 but with no activity

nyh commented 11 months ago

Is it the policy of this repository to close issues which can be moved (or are already) in the upstream repository? Do we regularly merge fixes from the upstream?

Lorak-mmk commented 11 months ago

I think we should leave this open - if upstream doesn't fix this soon (and I doubt they will) then we should do it

avikivity commented 11 months ago

Yes, let's not wait for them, this blocks users of Fedora 39.

avikivity commented 11 months ago

It can probably be worked around - asyncore is just one of the fallbacks. We can conditionally import it and not consider it if not available.

Lorak-mmk commented 11 months ago

It can probably be worked around - asyncore is just one of the fallbacks. We can conditionally import it and not consider it if not available.

The current code is here:

try:
    from cassandra.io.twistedreactor import TwistedConnection
except ImportError:
    TwistedConnection = None

try:
    from cassandra.io.eventletreactor import EventletConnection
except ImportError:
    EventletConnection = None

[...]

def _is_eventlet_monkey_patched():
    if 'eventlet.patcher' not in sys.modules:
        return False
    import eventlet.patcher
    return eventlet.patcher.is_monkey_patched('socket')

def _is_gevent_monkey_patched():
    if 'gevent.monkey' not in sys.modules:
        return False
    import gevent.socket
    return socket.socket is gevent.socket.socket

# default to gevent when we are monkey patched with gevent, eventlet when
# monkey patched with eventlet, otherwise if libev is available, use that as
# the default because it's fastest. Otherwise, use asyncore.
if _is_gevent_monkey_patched():
    from cassandra.io.geventreactor import GeventConnection as DefaultConnection
elif _is_eventlet_monkey_patched():
    from cassandra.io.eventletreactor import EventletConnection as DefaultConnection
else:
    try:
        from cassandra.io.libevreactor import LibevConnection as DefaultConnection  # NOQA
    except ImportError:
        from cassandra.io.asyncorereactor import AsyncoreConnection as DefaultConnection  # NOQA

So asyncore is used if gevent / eventlet / libev are not found. I see that Twisted is never imported as default connection - I wonder why, we should check if all tests pass with it. If it works properly, then perhaps it can be added as another fallback - but in order for the driver to work by default we would need to add non-optional dependency on twisted, which is not an optimal solution (driver strives to have minimal dependencies, currently only six and geomet)

avikivity commented 11 months ago

What's the designated replacement? asyncio?

avikivity commented 11 months ago

I see cassandra/io/asyncioreactor.py, can we use this instead?

Lorak-mmk commented 11 months ago

What's the designated replacement? asyncio?

Imho yes, that's what replaced asyncore in Python's stdlib so it should be a fine replacement for us.

I see cassandra/io/asyncioreactor.py, can we use this instead?

We can't because it doesn't work (and is even described as experimental), it needs to be fixed/finished first.

fruch commented 11 months ago

@Lorak-mmk

And this should be done in upstream, it's in no way Scylla specific.

we for sure can give them an hand with it, as we were doing in multiple cases, yes, making asyncio as the default might be a bit bigger than other fixes/changes we did so far.

fruch commented 11 months ago

and @jdavidagudelo, thanks for the report

Lorak-mmk commented 11 months ago

@Lorak-mmk

And this should be done in upstream, it's in no way Scylla specific.

we for sure can give them an hand with it, as we were doing in multiple cases, yes, making asyncio as the default might be a bit bigger than other fixes/changes we did so far.

Yeah, but I think in this case we can't wait for them - we need to fix it in our fork, and then provide the patches upstream to potentially reduce conflicts in the future.