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

Enable support for tornado 6.2 #7134

Closed
wants to merge 10 commits into from
Closed

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Oct 13, 2022

Most of the work in deprecation was done as part of #6680; with
tornado 6.2 in the few tests that still rely on creating an event loop
we must catch deprecation warnings from tornado in addition to those
from asyncio.

I suppose that the next step would be to remove support in
LoopRunner and friends for this deprecated functionality, and then
cull the tests that are checking for this behaviour, but I have not
done this here.

This ports over the version pin changes from #6991.

Closes #6669.

  • Tests added / passed
  • Passes pre-commit run --all-files

@@ -347,7 +347,7 @@ def key(ws):
adaptive = cluster.adapt(minimum=1)
await adaptive.adapt()

while len(cluster.scheduler.workers) == 4:
while len(cluster.scheduler.workers) >= 3:
Copy link
Contributor Author

@wence- wence- Oct 13, 2022

Choose a reason for hiding this comment

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

This is unrelated to the tornado changes, but this test was intermittently failing locally.

We're attempting to scale down from four to two workers (based on the grouping key, IIUC), but we might not have torn down both additional workers by the time this loop exits if just checking that we no longer have four workers.

I can spin this out into a separate PR if preferable.

@wence- wence- mentioned this pull request Oct 13, 2022
7 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±    0         15 suites  ±0   6h 32m 2s ⏱️ - 1m 12s
  3 224 tests  -     1    3 136 ✔️  -     6    84 💤 +  1    4 +  4 
24 524 runs  +679  23 542 ✔️ +605  960 💤 +52  22 +22 

For more details on these failures, see this check.

Results for commit ff6f673. ± Comparison against base commit 803c624.

♻️ This comment has been updated with latest results.

@wence-
Copy link
Contributor Author

wence- commented Oct 13, 2022

Windows fail appears to be flaky/bad test #7135;

@wence-
Copy link
Contributor Author

wence- commented Oct 18, 2022

Since I didn't manage to get this into 2022.10.2, I'd like to have a more comprehensive go this time.

As it stands, this PR just kicks the can down the road wrt deprecated functionality: distributed internally doesn't use the deprecated functionality so I only needed to touch tests.

Depending on whether we feel the deprecation cycle has been long enough (#6680 came in in July) the larger set of changes would be to remove all the deprecated functionality in LoopRunner and so forth, and then pull out the tests of this functionality completely.

Before I head in this direction, do we want that now?

@fjetter
Copy link
Member

fjetter commented Oct 20, 2022

I still see IOLoop.make_current being used in Nanny._run. This might be the final invocation in our actual code base.

If this is just about ripping out all deprecated calls concerning the loop runner, I'm OK with this. afaik, most downstream consumers were migrated. @jacobtomlinson I believe you were involved in these efforts.

@wence- wence- marked this pull request as draft November 2, 2022 11:22
@@ -5538,23 +5538,18 @@ async def test_future_auto_inform(c, s, a, b):
await asyncio.sleep(0.01)


@pytest.mark.filterwarnings("ignore:There is no current event loop:DeprecationWarning")
@pytest.mark.filterwarnings("ignore:make_current is deprecated:DeprecationWarning")
@pytest.mark.filterwarnings("ignore:clear_current is deprecated:DeprecationWarning")
def test_client_async_before_loop_starts(cleanup):
Copy link
Member

Choose a reason for hiding this comment

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

the name of the test doesn't match what's done in the body of the test anymore

@graingert
Copy link
Member

I still see IOLoop.make_current being used in Nanny._run. This might be the final invocation in our actual code base.

I opened #7240 to remove the calls from Nanny._run

tornado 6.2 deprecates functionality that relies on deprecated asyncio
calls (get_event_loop and friends); a few tests still use a pattern of
implicitly creating a new loop (and test that appropriate deprecation
warnings are uttered), so just catch these new tornado warnings for
now, until the functionality is removed in distributed.
requirements.txt Outdated Show resolved Hide resolved
@graingert graingert marked this pull request as ready for review November 9, 2022 12:09
@graingert graingert self-requested a review November 9, 2022 12:13
@wence-
Copy link
Contributor Author

wence- commented Nov 9, 2022

Let me fix the lint errors, but I am pretty sure this is not yet ready unfortunately.

@hayesgb hayesgb added the enhancement Improve existing functionality or make things work better label Nov 10, 2022
@graingert graingert mentioned this pull request Nov 16, 2022
2 tasks
@wence-
Copy link
Contributor Author

wence- commented Nov 23, 2022

FWIW, I've had a go at trying to understand how LoopRunners interact with the rest of the code and I'm overly confused about what the semantics should be. So if someone with a better understanding than me knows what to do here, I would encourage them to step in. The more recent changes turn the deprecation warnings in LoopRunner into runtimeerrors, so we can see where things break in the test suite.

@fjetter
Copy link
Member

fjetter commented Nov 24, 2022

closed and incorporated by #7286

@fjetter fjetter closed this Nov 24, 2022
@wence- wence- deleted the wence/fix/tornado-dep branch November 24, 2022 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or make things work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tornado 6.2 compatibility
5 participants