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

Update CI to Python 3.10 #33108

Closed
wants to merge 3 commits into from
Closed

Update CI to Python 3.10 #33108

wants to merge 3 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Mar 8, 2022

No description provided.

@foolip
Copy link
Member Author

foolip commented Mar 8, 2022

Some of the failures are because of from collections import Iterable, Mapping, which gives this warning in Python 3.8:

DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.9 it will stop working

@foolip
Copy link
Member Author

foolip commented Mar 8, 2022

This is the code in question:

https://github.com/python-hyper/hyper/blob/b77e758f472f00b098481e3aa8651b0808524d84/hyper/http11/connection.py#L13

hyper isn't being maintained any longer, so no chance of a fix in a new release.

@jgraham how do you think we should handle this? Do we patch our copy?

@foolip
Copy link
Member Author

foolip commented Mar 8, 2022

This will also be blocked by mozilla/sphinx-js#186.

@foolip
Copy link
Member Author

foolip commented Mar 22, 2022

@jgraham unfortunately I've hit another problem with Python versions here. To avoid building aioquic we need to pick versions that have prebuilt wheels for Python 3.6 and 3.10, but unfortunately I don't think such a version exist, although I haven't tested all versions between 0.9.15 and 0.9.19.

b292cb6 seems to have mostly worked, but not entirely, there are still Azure Pipelines failures where it looks like 0.9.15 is being attempted even though we should be running Python 3.10 there.

Any thoughts on how to deal with this situation? Options I can see:

  • Just allow the dependency to be built from source, installing all the stuff required to do that on all platforms
  • Stop running the webtransport tests when using Python 3.6
  • Figure out a way of installing different versions of aioquic depending on Python version that actually works

@jgraham
Copy link
Contributor

jgraham commented Mar 22, 2022

Just allow the dependency to be built from source, installing all the stuff required to do that on all platforms

I'm pretty sure that's going to be a "now you have N problems" approach. Also since upstream dropped 3.6 support there's no real guarantee it would actually solve the problem.

Stop running the webtransport tests when using Python 3.6

This seems plausible given that aioquic is already set up as an optional component (the server needs to be explcitly enabled), and afaik Gecko doesn't run these tests at the moment (I think we'd like to run them at some point, but I don't have any information to suggest that has to be before updating to 3.7).

Figure out a way of installing different versions of aioquic depending on Python version that actually works

I think the worry with that approach is that you could end up with results that depend on the version of aioquic installed. Or we could end up with interface differences. But if there was every a non-optional library for which we wanted to support more Python versions than the upstream, I think this would be the only workable approach, so we may end up needing to use it at some point.

@DanielRyanSmith
Copy link
Contributor

After doing some investigation, I think the reason the CI is not installing the correct aioquic version correctly for pipeline affected tests without changes: Safari Technology Preview because it is switching branches before it installs Python dependencies.

Screen Shot 2022-03-24 at 4 08 58 PM

Maybe I'm misunderstanding this, but it seems like we're not actually taking new code changes into consideration at this step

@foolip
Copy link
Member Author

foolip commented Mar 25, 2022

@DanielRyanSmith ah yes, of course, good find!

We do this because we want to run the affected tests without the changes, and we do this by simply checking out the commit that was merged to produce the commit under test.

But the version of Python installed is still going to be determined by .azure-pipelines.yml from the PR branch.

There are many ways of breaking things similar to this, and it's happened with Taskcluster too. But fortunately, once we know what the cause is, we can just admin merge.

"infrastructure/ tests: macOS" and "tools/wpt/ tests: macOS + Python 3.10" are still failing on Azure Pipelines though. In particular the latter seems like a real problem related to aioquic. Can you investigate?

When looking at a problem like this, I usually start by rebasing #28759 to be sure that all tests are in fact passing on master, to avoid a wild goose chase after a problem not caused by the PR. I'd suggest creating your own draft PR like it for these purposes.

(It would also be a good idea to automate this, so that we would find out if tests are ever broken on master.)

@foolip
Copy link
Member Author

foolip commented Apr 1, 2022

Abandoning this, @DanielRyanSmith will take over in a separate PR.

@foolip foolip closed this Apr 1, 2022
@foolip foolip deleted the py310 branch April 1, 2022 13:30
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.

3 participants