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

Requirements for Python dependency management #82

Open
foolip opened this issue May 4, 2021 · 17 comments
Open

Requirements for Python dependency management #82

foolip opened this issue May 4, 2021 · 17 comments

Comments

@foolip
Copy link
Member

foolip commented May 4, 2021

Hi @web-platform-tests/wpt-core-team!

Recently I have taken a closer look at how we track and install Python dependencies here in WPT. I wrote up two fairly detailed issues about what we're currently doing in web-platform-tests/wpt#28801 and web-platform-tests/wpt#28809.

I suspect that there's room to improve matters here, but want to gather requirements before I suggest something. But first, some problem I think are worth trying to solve:

  • Even though we vendor a lot of things, it's not enough to use ./wpt run (virtualenv is the additional dependency)
  • There's overlap between vendored and installed dependencies (e.g. aioquic, html5lib, pytest) and it's non-obvious if different versions are a problem or just fine
  • We don't pin all transient dependencies, which can (rarely) cause sudden breakage like in Unpin docutils in docs/requirements.txt wpt#28570
  • Updating tools/third_party/ is tricky, involving git subtree, and can't be merged like regular wpt PRs

Here are the requirements I would consider for any change here, roughly in order of importance:

  • Downstream users (Chromium, Gecko, WebKit) must be able to manage dependencies separately, avoiding any vendored or automatically downloaded dependencies in this repo. It should be easy to know which versions are used in wpt, but possible to diverge from that if necessary.
  • Users of ./wpt run should need at most one tool in addition to Python (currently Virtualenv) but zero is even better.
  • The distinction between direct and indirect dependencies should be clear, to avoid orphaned (unused) dependencies.
  • All dependencies, direct and indirect, should be pinned (think package-lock.json or Pipfile.lock)
  • Dependency updates should still be infrequent and deliberate, since they cause downstream work.
  • Installing dependencies and checking if they need to be updated should be fast.

I'm interested to hear if others have had trouble with dependencies, and if there are additional constraints/requirements here.

@LukeZielinski
Copy link
Contributor

Here's an overview of how wptrunner deps work on the Chromium side:

  • we are manually rolling tools (including a subset of tools/third_party) on a ~regular basis. During this process we may notice missing/stale deps and update appropriately (by also rolling another subdir of tools/third_party, or installing the dep)
  • we also have installed dependencies with pinned versions available for our infra. They don't come directly from pip but our own package server. The consequence of this is that it does take some work to get a new (or updated) package onto this server. This is currently a manual process.
  • when a particular dependency is both vendored and installed, our system prefers the installed one.

Chromium's use of wptrunner has been broken by new dependencies in the past, but it happens infrequently. We are almost always "behind" on versions. That is, we don't react to every single minor version update of deps. Instead, we roll up to the latest version of a package when something breaks (we get a chance to intervene without breaking the CI because we roll tools manually). Getting up to date is usually an easy process, although it can be slow due to the manual work involved to get a new package onto the package server. Transitive dependencies have been an issue (ie: you install one package but then realize you need a bunch more) -- this happened only once, though, as part of the py3 upgrade (which was a major change).

@foolip
Copy link
Member Author

foolip commented May 6, 2021

Thanks @LukeZielinski, that's super helpful!

Given "our own package server ... currently a manual process" would it be fair to say that it'd be an improvement if dependencies in WPT were updated rarely, on a very predictable schedule like quarterly? Right now we're landing pyup changes as they come in.

And am I right to assume that moving to only installed dependencies would be better, to not have to worry about vendored+installed compatibility and which takes precedence? (It seems to me that in WPT, the vendored deps will take precedence. Oops?)

@foolip
Copy link
Member Author

foolip commented May 6, 2021

web-platform-tests/wpt#24829 has some good discussion too.

@jgraham
Copy link
Contributor

jgraham commented May 6, 2021

On the gecko side:

  • We update the tools with the rest of the changes; there's no special handling
  • When stuff breaks on import it usually requires emergency patches. But I don't recall that being due to dependencies in particular (it's more often stuff like "wptrunner is broken in some configuration that we don't run on GitHub").
  • We don't really use the installed dependencies for anything in CI (they can be used by things that run locally). Indeed, to a reasonable approximation, we can't use PyPI dependencies in CI because we're using a mirror that requires manually updating packages and the process for that is laborious. There is talk about making it easier but there's no specific timeframe for that.
  • We are also a bit special because the mozbase and marionette packages canonically live in m-c and so we get the latest version of those rather than whatever's on PyPI. That's more normally a problem the other way around.

Basically for Gecko, at this time, moving toward more installed deps would be A Bad Thing, and in general we don't have loads of problems with the current setup where actually running tests can be done using entirely in-tree deps (at least when the tree is mozilla-central).

@LukeZielinski
Copy link
Contributor

Thanks @LukeZielinski, that's super helpful!

Given "our own package server ... currently a manual process" would it be fair to say that it'd be an improvement if dependencies in WPT were updated rarely, on a very predictable schedule like quarterly? Right now we're landing pyup changes as they come in.

I'd consider this an improvement yes. Mostly in terms of planning, so you could dedicate some time each quarter (or whatever) to get your deps figured out, and not have to scramble.

And am I right to assume that moving to only installed dependencies would be better, to not have to worry about vendored+installed compatibility and which takes precedence? (It seems to me that in WPT, the vendored deps will take precedence. Oops?)

All-vendored or all-installed both seem better than the hybrid thing that exists today. One argument I could see against all-vendored deps is that it checks a whole pile of random code into the tree (pollutes codesearch results for instance). Although we've never tried to go down this path, so there could be technical challenges I'm not aware of.

@foolip
Copy link
Member Author

foolip commented May 7, 2021

Thanks @jgraham, that's very helpful!

In case you saw it, I want to be clear that the all-installed web-platform-tests/wpt#28796 isn't really the direction I'm hoping for here, that's was an exploration to figure out what the dependencies actually are.

It sounds like a requirements from Gecko is that it should be easy import WPT and all of its dependencies so that everything needed is in-tree, and no network access is needed while running WPT in Gecko. Having precisely the dependencies that Gecko doesn't already have manually vendored into WPT meets that requirement.

A rough sketch of the direction I am hoping for now:

  • all direct and indirect dependencies tracked, with versions and metadata like "needed for ./wpt manifest" at some granularity
  • Gecko, Chromium and WebKit can easily compute the subset of the dependencies they need, and either put them into their own package servers, put them into the tree, or something else
  • We could keep a tools/third_party/ in-tree which is exactly what Gecko needs, if it comes to that
  • ./wpt run still just does the right thing for users (and maybe ./wpt install can also install all needed dependencies for later ./wpt run --i-am-offline)

@jgraham
Copy link
Contributor

jgraham commented May 7, 2021

So one reasonable goal here would be to remove localpaths.py. I wonder if something like the following could work:

  • We have a requirements_vendored.txt file that installs everything under third_party into the virtualenv by path.
  • We additionally have a requirements_thirdparty.txt that lists the specific versions of those depencies that we're using
  • We have a job that fails in CI if the vendored version doesn't match the specified version

That would have the following properties:

  • dependabot or pyupbot could be used to alert us when something in third_party is updated.
  • It would be possible for users to use either the vendored dep or the in-tree one at their preference, knowing that both are the same version (this does change if we have local patches; idk if that's a current problem).

@foolip
Copy link
Member Author

foolip commented May 7, 2021

Getting rid of localpaths.py is indeed an appealing part of this.

It would be possible for users to use either the vendored dep or the in-tree one at their preference, knowing that both are the same version

Yes!

this does change if we have local patches; idk if that's a current problem

I looked at the recent commits of everything in web-platform-tests/wpt#28801 and didn't find anything with local modifications. Only pywebsocket3 is problematic because it's not yet available on pypi, see GoogleChromeLabs/pywebsocket3#30.

@jgraham
Copy link
Contributor

jgraham commented May 7, 2021

One concern here is startup latency; if every wpt run call is invoking pip and checking that all the packages are installed it's going to add noticable startup time.

@gsnedders
Copy link
Member

For WebKit, there's a few notable cases:

  1. Running LayoutTests, which in the future should require wpt.manifest
  2. Running check-style, which in the future should require wpt.lint
  3. Running WebDriverTests, which requires wptrunner etc. (though I think safaridriver might not use wptrunner, merely using pytest directly?)

Historically, the goal was for at least ./wpt manifest and ./wpt lint to be runnable without any external dependencies (though I'm not sure how much value there is in the former?), but we seem to have gone beyond that (e.g., with hyper and h2). One of the problems with the status-quo is I think it's unclear why things are vendored v. put in as a requirement?

At least from my point-of-view, I'd rather we avoid the status-quo where we have our tools adding our own versions of the packages to sys.path, rather than how other tools (e.g., pip) do vendoring whereby they end up with imports like from pip._vendored import html5lib. That is probably the most essential thing to do.

To note, I think pytest might be the only thing we vendor which vaguely regularly has any API breakage? Maybe h2 too?

All-vendored or all-installed both seem better than the hybrid thing that exists today. One argument I could see against all-vendored deps is that it checks a whole pile of random code into the tree (pollutes codesearch results for instance). Although we've never tried to go down this path, so there could be technical challenges I'm not aware of.

One problem is that some of the packages that wptrunner requires depend on compiled extensions (such as Pillow). We need to install them somewhere as a result.

@foolip
Copy link
Member Author

foolip commented May 7, 2021

One concern here is startup latency; if every wpt run call is invoking pip and checking that all the packages are installed it's going to add noticable startup time.

That's precisely one of the things I'd like to resolve here. When I run ./wpt run myself, I can tell that pip is being called at least twice based on the deprecation messages. This doesn't happen in vendor CI, of course, but I'd like to get a better faster experience in the CLI too :)

@gsnedders
Copy link
Member

Commands we have left which don't use a venv:

Command Script Function
./wpt test-jobs ./tools/ci/jobs.py run
./wpt make-hosts-file ./tools/ci/make_hosts_file.py run
./wpt regen-certs ./tools/ci/regen_certs.py run
./wpt docker-build ./tools/docker/frontend.py build
./wpt docker-run ./tools/docker/frontend.py run
./wpt manifest-download ./tools/manifest/download.py run
./wpt spec ./tools/manifest/spec.py run
./wpt test-paths ./tools/manifest/testpaths.py run
./wpt manifest ./tools/manifest/update.py run
./wpt serve ./tools/serve/serve.py run
./wpt create ./tools/wpt/create.py run
./wpt rev-list ./tools/wpt/revlist.py run_rev_list
./wpt branch-point ./tools/wpt/testfiles.py display_branch_point
./wpt files-changed ./tools/wpt/testfiles.py run_changed_files
./wpt tests-affected ./tools/wpt/testfiles.py run_tests_affected

And I believe what we have vendored that we directly use is:

atomicwrites==1.1.5
h2==4.1.0
html5lib==1.1
mozilla-tooltool-client @ git+https://github.com/mozilla-releng/tooltool.git@c1bda97eda1ec5246aa73a358c5e1d47e222380c#subdirectory=client
pywebsocket3==4.0.2
pytest==8.2.1
pytest-asyncio==0.19.0
websockets==12.0

Note that pywebsocket3 we have several patches on, and mozlog which we have more clearly forked (maybe? web-platform-tests/wpt#48948).

Especially with venv now depended upon, we should probably whether it actually makes sense to have much vendored at all.

@gsnedders
Copy link
Member

I do very much wonder the value of having mozilla-tooltool-client/pytest/pytest-asyncio/websockets vendored actually makes sense — they're only used by wptrunner, and that's always had its own dependencies?

That leaves atomicwrites and html5lib used by manifest, and h2 and pywebsocket3 used by wptserve.

@jgraham
Copy link
Contributor

jgraham commented Nov 7, 2024

It's a little bit complicated by the fact that mozilla-central is in the unique position of having mozbase packages already in-tree, so we end up with all the deps required to run wptrunner available without depending on PyPI. So changes here do have a material impact for us.

@gsnedders
Copy link
Member

It's a little bit complicated by the fact that mozilla-central is in the unique position of having mozbase packages already in-tree, so we end up with all the deps required to run wptrunner available without depending on PyPI. So changes here do have a material impact for us.

You also have all the other non-mozbase packages requires to run wptrunner, though? It's not like these are the first non-mozbase dependencies.

@jgraham
Copy link
Contributor

jgraham commented Nov 7, 2024

Well the point is that in CI the only wpt-specific thing we install from a package is aioquic, and that's caused ongoing problems because we need to update the internal mirror which currently isn't a very streamlined process. For things like pytest, we have vendored copies already, but they aren't necessarily the same version we're using in wpt. So moving to vendoring ~nothing in wpt isn't impossible, but neither is it easy for us.

@gsnedders
Copy link
Member

gsnedders commented Nov 7, 2024

To make concrete one of my motivations for caring here: I want to be able to import tools.manifest from outside of WPT (and really the use of tools as the top-level package scares me slightly, because that seems relatively likely to conflict), without also ending up with all of WPT's vendored versions ending up on my path.

web-platform-tests/wpt#49034 is one possible path forward here (moving to importing everything from tools.third_party.XXX), but it will almost certainly break all of our WebDriver tests (as the tests all import from pytest, which is no longer guaranteed to be installed). If we migrate all the tests to use tools.third_party.pytest, this further complicates running them with pytest directly outside of our tree.

I guess we could do sys.modules hackery to expose it as pytest in tools.wptrunner.wptrunner.executors.pytestrunner.runner?

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

No branches or pull requests

4 participants