fix(tests): download the shared torchvision checkpoints before pytest forks - #4587
Open
shoumikhin wants to merge 2 commits into
Open
fix(tests): download the shared torchvision checkpoints before pytest forks#4587shoumikhin wants to merge 2 commits into
shoumikhin wants to merge 2 commits into
Conversation
… forks ts-api is red on Windows on most main runs: FAILED api/test_operator_fallback.py::TestFallbackModels::test_fallback_mobilenet_v2 _pickle.UnpicklingError: pickle data was truncated pytest runs one test file per xdist worker (addopts carries --dist=loadfile), and api/test_operator_fallback.py and api/test_module_fallback.py both open with models.mobilenet_v2(pretrained=True). Two workers therefore download the same checkpoint at the same time. torch.hub finishes a download with shutil.move, which calls os.rename and falls back to a plain copy when that raises. On Linux os.rename over an existing file succeeds and is atomic; on Windows it raises FileExistsError, so the second worker copies over the file the first one is reading and torch.load sees a truncated pickle. That is why this only ever fails on Windows, and why the config that fails moves around between runs. resnet18 is in the same position: three files in api, two in models and two in integrations. The ts suites already run tests/modules/hub.py as a setup step to put what they need on disk before pytest starts. It just does not cover the torchvision checkpoints, so fetch those there too. One process, no concurrent writers, and the workers only ever read. Checkpoints only one file uses cannot race, so they are left alone.
The repository lint job runs `black --check .` across the whole tree, so any file that does not match the formatter fails CI for every open pull request, not only the one that touched it. `tests/py/dynamo/conversion/test_cumsum_aten.py` is currently not black-conformant on main, which turns the Python Linting check red here. Reformat that one file with black. This is a formatting-only change: two statements that fit on a single line are un-wrapped. No test logic changes. Verified by running `black --check .` on the full tree: all files pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ts-apion Windows is red on most main runs, one or two configs at a time, and the configs move around:Other runs show
invalid load key, '\x00'for the same test. Linux never fails this way.Cause
addoptsinpyproject.tomlcarries--dist=loadfile, so pytest gives each test file its own xdist worker.api/test_operator_fallback.pyandapi/test_module_fallback.pyboth open withmodels.mobilenet_v2(pretrained=True), so two workers download the same checkpoint at the same time.torch.hub.download_url_to_filewrites to a temporary file and finishes withshutil.move.shutil.movecallsos.renameand falls back to a plain copy when that raises. On Linuxos.renameover an existing file succeeds and is atomic. On Windows it raisesFileExistsError, so the fallback copy rewrites the file in place while the other worker is reading it, andtorch.loadsees a truncated pickle. That is why the failure is Windows only and why it lands on a different config each run.resnet18is in the same position: three files inapi/, two inmodels/, two inintegrations/.Fix
The three
tests/py/tssuites already runtests/modules/hub.pyas a setup step to put what they need on disk before pytest starts. It does not cover the torchvision checkpoints, so fetch those there as well. One process, no concurrent writers, and the workers only ever read. Checkpoints that only one file uses cannot race, so they are left alone. A download failure here is reported and ignored, because warming the cache is an optimization and the tests still fetch what they need.Testing
The prefetch runs only in the
hubsetup step, which the CI logs show already executing forts-api. Windows CI is the place this can be confirmed; the failure needs two workers racing, which does not reproduce on Linux.