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

Upgrade to tornado 6.2 #7286

Merged
merged 3 commits into from
Nov 24, 2022
Merged

Upgrade to tornado 6.2 #7286

merged 3 commits into from
Nov 24, 2022

Conversation

graingert
Copy link
Member

@graingert graingert commented Nov 10, 2022

Closes #6669.

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 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 36m 59s ⏱️ - 4m 28s
  3 225 tests ±0    3 141 ✔️ +3    84 💤 ±0  0  - 3 
23 845 runs  ±0  22 936 ✔️ +3  909 💤 ±0  0  - 3 

Results for commit 82fc870. ± Comparison against base commit 2963bf1.

♻️ This comment has been updated with latest results.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @graingert. cc'ing @wence- into this PR to make sure we're coordinating with #7134, which looks very similar

@graingert graingert force-pushed the tornado-62 branch 2 times, most recently from d0ce45b to fd74414 Compare November 16, 2022 14:00
@graingert graingert marked this pull request as ready for review November 16, 2022 14:49
@graingert
Copy link
Member Author

Thanks @graingert. cc'ing @wence- into this PR to make sure we're coordinating with #7134, which looks very similar

this is the code from #7134 but without the looprunner changes, because they're not strictly needed for 6.2 support - but they are desirable soon - maybe in the next release?

@wence-
Copy link
Contributor

wence- commented Nov 16, 2022

this is the code from #7134 but without the looprunner changes, because they're not strictly needed for 6.2 support - but they are desirable soon - maybe in the next release?

Thanks @graingert for factoring this out, I am hoping to come back to #7134 next week.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

this is the code from #7134 but without the looprunner changes, because they're not strictly needed for 6.2 support - but they are desirable soon - maybe in the next release?

Thanks @graingert, that context is useful. I just saw multiple PRs, so wanted to make sure we were all on the same page.

@@ -38,7 +38,7 @@ dependencies:
- sortedcollections
- tblib
- toolz
- tornado<6.2
- tornado >=6.2
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the version restriction we want, given what's specified in requirements.txt

Suggested change
- tornado >=6.2
- tornado >=6.0.3

Copy link
Member Author

Choose a reason for hiding this comment

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

if the restriction doesn't require >=6.2 in the environment then it follows what's currently released for the distributed package

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem we're having here is that all python versions are now testing against 6.2 but we're not testing against 6.0.3 anymore, are we?

I'm fine with this for now if we add <6.2 to #7285

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem we're having here is that all python versions are now testing against 6.2 but we're not testing against 6.0.3 anymore, are we?

Yeah, that's what I was wanting to avoid.

I'm fine with this for now if we add <6.2 to #7285

Yep, I think the mindeps build will be the long term answer for testing against our lowest supported minimum version.

FWIW I didn't mean for this comment to be majorly blocking. @graingert if you and @fjetter are happy with things as they are, that's fine by me.

@jrbourbeau jrbourbeau changed the title upgrade to tornado 6.2 Upgrade to tornado 6.2 Nov 16, 2022
graingert and others added 3 commits November 23, 2022 10:29
Co-Authored-By: Lawrence Mitchell <[email protected]>
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.
Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

How did you pull this off?
image

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.

Tornado 6.2 compatibility
4 participants