Support the deprecation of asyncore in python 3.12 #260

fruch · 2023-10-11T12:30:09Z

since we need to deprecate asyncore which was the default event loop manager, we need to extend the testing of some of the other so we can select a new default

fruch · 2023-10-11T12:31:42Z

@Lorak-mmk
started tweaking the CI a bit so we could cover other event loops
and test the python 3.12 support (as we'll start addressing it)

.github/workflows/integration-tests.yml

fruch · 2023-10-12T07:03:55Z

@Lorak-mmk

FYI, the situation with asyncio on python-3.8 is o.k.

One extra failing test

We'll need to test more python versions

fruch · 2023-10-12T07:07:38Z

I'm setting a goal to this PR (so won't spread too much)

CI working for 3.8 - asyncore/libev/asyncio

3.12 - only libev

Other python versions aren't mandatory
for now

Lorak-mmk · 2023-10-12T07:12:53Z

@Lorak-mmk

FYI, the situation with asyncio on python-3.8 is o.k.

One extra failing test

We'll need to test more python versions

Wow, I'm very surprised that it works

fruch · 2023-10-17T21:20:21Z

I'm setting a goal to this PR (so won't spread too much)

CI working for 3.8 - asyncore/libev/asyncio

3.12 - only libev

Other python versions aren't mandatory for now

so at the end I've did much more in this PR
and it seems to have the full support for asyncio working and passing integration tests
and python3.12 to be workable, i.e. we can use asyncio and libev, which sound good enough for me.

followup PRs would be

wheel building for python 3.12
testing the extra loop managers, they seem to be having issue, and we'll need to pick on which version to test them regularly

fruch · 2023-10-17T21:27:14Z

@Lorak-mmk

from my POV we can merge this one, once I get a green CI

but I still want you to take a look at the actual commit (not the CI ones, they are quite straightforward)

fruch · 2023-10-18T21:21:25Z

#264 seems to be flaky, and not sure exactly why...
I'lll leave it for now,

@Lorak-mmk this one is ready from my POV

Lorak-mmk

Requested some changes. We should probably offer this PR upstream after we merge it,

cassandra/cluster.py

Lorak-mmk · 2023-10-18T12:44:04Z

tests/integration/standard/test_connection.py

This file has a ConnectionTests class which is then subclassed (AsyncoreConnectionTests and LibevConnectionTests). It would be good if a part of this PR was to add subclasses for other reactors. If that's too much work, a TODO will suffice.
I also wonder if a subclass for each reactor is the best approach - it causes us to run all of them always, no matter what is the value of EVENT_LOOP_MANAGER, maybe it should be conditional?

One more thing, in case of import error, relevant var is set to None - I don't get why it doesn't cause tests to fail - I don't see any None checks, and ConnectionTests.setUp calls self.klass.initialize_reactor() which should fail - so there is something I don't understand in this code...

Lorak-mmk · 2023-10-18T12:50:18Z

tests/integration/standard/test_scylla_cloud.py

 # need to run them with specific configuration like `gevent.monkey.patch_all()` or under async functions
-unsupported_connection_classes = [GeventConnection, AsyncioConnection, EventletConnection]
+# unsupported_connection_classes = [GeventConnection, AsyncioConnection, EventletConnection]


I don't immediately see why wouldn't AsyncioConnection work here. Did you try to run this test with it?
Also, is commenting out this line and 3 imports necessary?

Asyncio wasn't working at all when those tests were written, so I didn't introduce it now

As for the comment, I rather not import the unused classes, it was for documenting only, and import of some of them now complicates things

.github/workflows/integration-tests.yml

Lorak-mmk · 2023-10-24T13:10:10Z

cassandra/io/asyncioreactor.py

+        loop_args = dict()
+        if sys.version_info[0] == 3 and sys.version_info[1] < 10:
+            loop_args['loop'] = self._loop
+        self._write_queue = asyncio.Queue(**loop_args)
+        self._write_queue_lock = asyncio.Lock(**loop_args)


I really don't like such conditionals...
This could be solved differently - before 3.10, when loop argument is not provided, current loop is used (fetched during __init__). So we could put a task that creates those objects on a loop and get the result, something like this (it can most likely be a bit prettier, it's just a PoC):

async def asyncify(f, *args, **kwargs): return f(*args, **kwargs) self._write_queue = asyncio.run_coroutine_threadsafe(asyncify(asyncio.Queue)(), self._loop).result() self.._write_queue_lock = asyncio.run_coroutine_threadsafe(asyncify(asyncio.Lock)(), self._loop).result()

However I'm not really sure it's better than (if python < 3.10) version...
If we stay with if approach, let's add a TODO to remove it after dropping support for 3.9

seems like in upstream they removed completely
datastax@e9136f4
which is quite weird, since it's break python 3.8

Sorry, missed this one earlier. The params in question were removed in PYTHON-1313. There's more detail in the ticket but in short asyncio will default to the event loop for the current thread since 3.7. And since the asyncio reactor explicitly sets the created loop for the current thread via set_event_loop() immediately after it's creation there didn't appear to be any obvious need to pass it around.

The relevant tests pass on Python 3.8 on our Jenkins instance, but if I've missed something there I'm certainly open to changes.

again, from my own local tests, and tests running on github actions,
python3.8 break totally when those parameters are not passed to the locks

cassandra/io/asyncioreactor.py

Lorak-mmk · 2023-10-24T13:26:49Z

tests/integration/cqlengine/model/test_model.py

relevant_warnings can probably be made empty if we apply my suggestion instead of version-conditional code.

Lorak-mmk · 2023-10-24T13:27:38Z

cassandra/cluster.py

-        from cassandra.io.asyncorereactor import AsyncoreConnection as DefaultConnection  # NOQA
+        from cassandra.io.asyncioreactor import AsyncioConnection as DefaultConnection  # NOQA


We could also think about removing asyncore in a follow up PR.

There's no rush on that,

And we can sync with upstream about when todo it, i.e. not to break current users that might have hardcore it

absurdfarce · 2023-11-06T19:59:07Z

Hey all, just a quick note on upstream plans related to what you guys are working on for this ticket.

We're gearing up for a 3.29.0 release which aims to follow our policy of supporting all non-EOL releases at time of release. That means we're aiming to support 3.8 through 3.12 with 3.29.0. The removal of asyncore in 3.12 is being pursued in PYTHON-1366. I had originally intended to make the asyncio reactor the default (replacing asyncore) for 3.29.0 but I'm not convinced the asyncio reactor is stable enough to support that yet. So for 3.29.0 I'm settling on messaging telling users to use one of the third-party event loops for now; asyncio will become the default once we can make it more stable.

To that end: if you guys are inclined to contribute any fixes to the asyncio reactor upstream we'd be more than happy to take a look at them. It's probably worthwhile to separate that work from any work around 3.12 compat, though, in order to avoid any confusion of goals.

Comments welcome. :)

fruch · 2023-11-07T05:48:16Z

Hey all, just a quick note on upstream plans related to what you guys are working on for this ticket.

We're gearing up for a 3.29.0 release which aims to follow our policy of supporting all non-EOL releases at time of release. That means we're aiming to support 3.8 through 3.12 with 3.29.0. The removal of asyncore in 3.12 is being pursued in PYTHON-1366. I had originally intended to make the asyncio reactor the default (replacing asyncore) for 3.29.0 but I'm not convinced the asyncio reactor is stable enough to support that yet. So for 3.29.0 I'm settling on messaging telling users to use one of the third-party event loops for now; asyncio will become the default once we can make it more stable.

I've managed at least to get the asyncio passing all the integration suite

Would love to hear what are the other thing you think would be need to title it ready for production (maybe we help in some parts)

To that end: if you guys are inclined to contribute any fixes to the asyncio reactor upstream we'd be more than happy to take a look at them. It's probably worthwhile to separate that work from any work around 3.12 compat, though, in order to avoid any confusion of goals.

Yeah I know I was trying to do multiple things here, but I'll can separate the asyncio only change into an upstream PR.

Comments welcome. :)

fruch · 2023-11-07T05:49:04Z

Requested some changes. We should probably offer this PR upstream after we merge it,

Yes, we probably would need to break it a bit

absurdfarce · 2023-11-13T15:33:54Z

Hey @fruch , appreciate the follow-up!

An update to my previous comment: I've now merged the changes in PYTHON-1366 into the code base. As mentioned above these changes throw exceptions when trying to use asyncore with Python 3.12. These warnings coupled with documentation and messaging around the release should make clear why asyncore isn't supported on Python 3.12 and what the user can do about it (i.e. use another event loop).

I decided it was easier to take an incremental approach like this (not failing completely when trying to use asyncore with Python 3.12 while also providing clear instructions) in the hope that it would be less intrusive for our users. Which brings me to your other question: what do we need to see to make asyncio the default?

I'm impressed with the work you guys have done to get asyncio passing all the integration tests and would love to incorporate those changes into upstream as well. Other than that I'd like to see how this reactor behaves over time. We typically run "duration tests" with each release that run for several days in order to identify any regressions that only appear over time. I'd like to see an asyncio reactor pass such a test. I'd also like to see some more load testing, just to make sure it won't have any issues if we stress it a bit.

I'm not at all opposed to making asyncio the default reactor for future driver releases; with the removal of asyncore in 3.12 that seems like the correct way to go. I just want to do it in a step-wise fashion so that we don't drop a ton of change on our users within a single release.

Thanks again!

fruch · 2023-11-13T16:05:06Z

Hey @fruch , appreciate the follow-up!

An update to my previous comment: I've now merged the changes in PYTHON-1366 into the code base. As mentioned above these changes throw exceptions when trying to use asyncore with Python 3.12. These warnings coupled with documentation and messaging around the release should make clear why asyncore isn't supported on Python 3.12 and what the user can do about it (i.e. use another event loop).

I did thought we might be able to default to asyncio only once asyncore isn't available,
cause we are using that in cqlsh that we bundle with scylla (and I think cassandra does exactly the same)
it would break, since it's using the pure python implementation without any compilation
i.e. that default ux of be able to run with any changes, or selecting between the different options

I decided it was easier to take an incremental approach like this (not failing completely when trying to use asyncore with Python 3.12 while also providing clear instructions) in the hope that it would be less intrusive for our users. Which brings me to your other question: what do we need to see to make asyncio the default?

I'm impressed with the work you guys have done to get asyncio passing all the integration tests and would love to incorporate those changes into upstream as well. Other than that I'd like to see how this reactor behaves over time. We typically run "duration tests" with each release that run for several days in order to identify any regressions that only appear over time. I'd like to see an asyncio reactor pass such a test. I'd also like to see some more load testing, just to make sure it won't have any issues if we stress it a bit.

Yeah I thought that would helpful to grant trust, we didn't touch those test in this repo yet
one other idea I has was to use our dtest repo, and run it with asyncio, and flush out possible issues.

I'm not at all opposed to making asyncio the default reactor for future driver releases; with the removal of asyncore in 3.12 that seems like the correct way to go. I just want to do it in a step-wise fashion so that we don't drop a ton of change on our users within a single release.

that's great, that things are moving fast, having python 3.12 support is very important to us
our users (i.e. developers) are very keen on running latest versions of their linux distros
and python 3.12 is coming in the next version of a lot of the distros (and I think already know that)

Thanks again!

to conclude, I think we won't be merging those change, and we'll wait for the upstream release (which I hope is just around the corner)

as for the asyncio related changes, please see: #271

running on python 3.12, we get this error, we should ignore it until eventlet is fixed ``` ImportError while loading conftest '/home/runner/work/python-driver/python-driver/tests/integration/conftest.py'. tests/integration/__init__.py:16: in <module> from cassandra.cluster import Cluster cassandra/cluster.py:103: in init cassandra.cluster from cassandra.io.eventletreactor import EventletConnection cassandra/io/eventletreactor.py:18: in <module> import eventlet .test-venv/lib/python3.12/site-packages/eventlet/__init__.py:17: in <module> from eventlet import convenience .test-venv/lib/python3.12/site-packages/eventlet/convenience.py:7: in <module> from eventlet.green import socket .test-venv/lib/python3.12/site-packages/eventlet/green/socket.py:21: in <module> from eventlet.support import greendns .test-venv/lib/python3.12/site-packages/eventlet/support/greendns.py:45: in <module> from eventlet.green import ssl .test-venv/lib/python3.12/site-packages/eventlet/green/ssl.py:25: in <module> _original_wrap_socket = __ssl.wrap_socket E AttributeError: module 'ssl' has no attribute 'wrap_socket' ``` Ref: eventlet/eventlet#812

this isn't being used anyhow, and breaking support for python 3.12

since asyncore isn't available in python 3.12, we should be gracfully handle it, and enable any other event loop implementions to work

since python 3.12 is deprecating asyncore, we should make asyncio the default fallback event loop when asyncore isn't available asyncio now that it's fixed and we verified it's working (passing the integration suite) in multiple python versions we support (from 3.8 - 3.12)

fruch · 2023-11-20T22:14:10Z

@Lorak-mmk
Can you take a look at this one, this would enable us to release python3.12 wheel

And then I think we should release one last minor release, before syncing with upstream 3.28.0, or 3.29.0 once it would be available

.github/workflows/build-experimental.yml

.github/workflows/build-push.yml

Lorak-mmk · 2023-11-23T12:09:14Z

I left 2 questions in the comments, apart from that looks good to me. However I'd prefer to not release until #271 (comment) is addressed

fruch requested a review from Lorak-mmk October 11, 2023 12:30

fruch force-pushed the make_asyncore_unused branch 5 times, most recently from 32919c0 to a9bad78 Compare October 11, 2023 15:47

Lorak-mmk reviewed Oct 11, 2023

View reviewed changes

.github/workflows/integration-tests.yml Outdated Show resolved Hide resolved

fruch mentioned this pull request Oct 11, 2023

Eventlet Python 12.0 issue eventlet/eventlet#812

Closed

fruch force-pushed the make_asyncore_unused branch from ec80814 to 58f3ba1 Compare October 11, 2023 16:26

fruch force-pushed the make_asyncore_unused branch 12 times, most recently from fc7e475 to ebd44d3 Compare October 17, 2023 21:16

fruch force-pushed the make_asyncore_unused branch from ebd44d3 to 85a7211 Compare October 17, 2023 21:21

fruch requested a review from Lorak-mmk October 17, 2023 21:25

fruch mentioned this pull request Oct 18, 2023

[unstable][asyncio] test_execute_query_timeout is unstable #263

Closed

fruch force-pushed the make_asyncore_unused branch from b93369c to 48399d6 Compare October 18, 2023 21:20

Lorak-mmk requested changes Oct 24, 2023

View reviewed changes

fruch mentioned this pull request Nov 2, 2023

Remove some remnants of Python2 datastax/python-driver#1172

Merged

fruch changed the title ~~CI: run integration tests on multiple EVENT_LOOP_MANAGER and python versions~~ Support the depreciation of asyncore in python 3.12 Nov 9, 2023

fruch force-pushed the make_asyncore_unused branch from 48399d6 to 3a5609d Compare November 9, 2023 10:36

mykaul changed the title ~~Support the depreciation of asyncore in python 3.12~~ Support the deprecation of asyncore in python 3.12 Nov 9, 2023

fruch force-pushed the make_asyncore_unused branch 3 times, most recently from 926ae2d to 8bf2590 Compare November 9, 2023 22:31

fruch requested a review from Lorak-mmk November 19, 2023 16:33

fruch added 5 commits November 19, 2023 18:34

test_cluster: remove import asyncore

ed2b244

this isn't being used anyhow, and breaking support for python 3.12

handle the case asyncore isn't available

34037ed

since asyncore isn't available in python 3.12, we should be gracfully handle it, and enable any other event loop implementions to work

CI: enable builds of python 3.12 wheels

6730c67

fruch force-pushed the make_asyncore_unused branch from 8bf2590 to 6730c67 Compare November 19, 2023 16:36

Lorak-mmk mentioned this pull request Nov 19, 2023

io.asyncioreactor: fix deprecated usages for working with python>=3.10 #271

Merged

Lorak-mmk approved these changes Nov 23, 2023

View reviewed changes

Lorak-mmk reviewed Nov 23, 2023

View reviewed changes

.github/workflows/build-experimental.yml Show resolved Hide resolved

.github/workflows/build-push.yml Show resolved Hide resolved

roydahan assigned fruch Nov 26, 2023

fruch merged commit 1cc6ccc into scylladb:master Nov 27, 2023
20 checks passed

Lorak-mmk mentioned this pull request Jun 4, 2024

AsyncioConnection: fix initialize_reactor when called in event loop #327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the deprecation of asyncore in python 3.12 #260

Support the deprecation of asyncore in python 3.12 #260

fruch commented Oct 11, 2023

fruch commented Oct 11, 2023

fruch commented Oct 12, 2023

fruch commented Oct 12, 2023

Lorak-mmk commented Oct 12, 2023

fruch commented Oct 17, 2023

fruch commented Oct 17, 2023

fruch commented Oct 18, 2023

Lorak-mmk left a comment

Lorak-mmk Oct 18, 2023

Lorak-mmk Oct 18, 2023

fruch Nov 7, 2023

Lorak-mmk Oct 24, 2023

fruch Nov 9, 2023

absurdfarce Nov 13, 2023

fruch Nov 13, 2023

Lorak-mmk Oct 24, 2023

Lorak-mmk Oct 24, 2023

fruch Nov 7, 2023

absurdfarce commented Nov 6, 2023

fruch commented Nov 7, 2023

fruch commented Nov 7, 2023

absurdfarce commented Nov 13, 2023 •

edited

Loading

fruch commented Nov 13, 2023

fruch commented Nov 20, 2023

Lorak-mmk commented Nov 23, 2023

		from cassandra.io.asyncorereactor import AsyncoreConnection as DefaultConnection # NOQA
		from cassandra.io.asyncioreactor import AsyncioConnection as DefaultConnection # NOQA

Support the deprecation of asyncore in python 3.12 #260

Support the deprecation of asyncore in python 3.12 #260

Conversation

fruch commented Oct 11, 2023

fruch commented Oct 11, 2023

fruch commented Oct 12, 2023

fruch commented Oct 12, 2023

Lorak-mmk commented Oct 12, 2023

fruch commented Oct 17, 2023

fruch commented Oct 17, 2023

fruch commented Oct 18, 2023

Lorak-mmk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absurdfarce commented Nov 6, 2023

fruch commented Nov 7, 2023

fruch commented Nov 7, 2023

absurdfarce commented Nov 13, 2023 • edited Loading

fruch commented Nov 13, 2023

fruch commented Nov 20, 2023

Lorak-mmk commented Nov 23, 2023

absurdfarce commented Nov 13, 2023 •

edited

Loading