test: add shared gloo process pool for distributed loss tests#1983
Draft
saud5150 wants to merge 3 commits into
Draft
test: add shared gloo process pool for distributed loss tests#1983saud5150 wants to merge 3 commits into
saud5150 wants to merge 3 commits into
Conversation
The raw torch.distributed.all_reduce overwrote the cross-correlation matrix in place and was not registered in the autograd graph. Under DDP gradient averaging this scaled the backbone gradient down by 1/world_size relative to single-GPU Barlow Twins on the global batch. Swap it for the autograd-aware torch.distributed.nn.all_reduce, whose backward is itself an all_reduce, so the gradient is scaled back up by world_size. The forward value is unchanged. Fixes lightly-ai#1977. Same fix as SIGReg in lightly-ai#1923.
Mocking all_reduce only tested the mock, not the real collective, and would break on any change to the reduction. Real multi-rank coverage is tracked in lightly-ai#1982.
Distributed loss tests either lived in a separate module disconnected from each loss's tests, or mocked torch.distributed - which only tests the mock, not the real collective (the gap that hid lightly-ai#1977 until the mock tests were reverted in lightly-ai#1980). Add a torchmetrics-style session-scoped gloo (CPU) process pool, started once per session and reused, gated behind USE_PYTEST_POOL (default off, not enabled in CI). Tests opt in with @pytest.mark.DDP + skipif. First consumer: two real 2-rank BarlowTwins tests; the gradient test is a regression test for lightly-ai#1977 and fails on the raw dist.all_reduce. The pool uses spawn, whose workers re-import the test module and deadlock under pytest's default prepend import mode; set --import-mode=importlib in addopts. Adapted from torchmetrics (Apache-2.0), tests/unittests/conftest.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Part of #1982 (PR 1 — draft for early feedback)
Description
Distributed loss behaviour is currently tested either in a separate module
(
tests/utils/test_dist__gather__losses.py) disconnected from each loss's owntests, or by mocking
torch.distributed— which only exercises the mock, not thereal collective (exactly the gap that hid #1977 until the mock tests were reverted
in #1980).
This adds a torchmetrics-style session-scoped gloo (CPU) process pool, started
once per session and reused, so a test can submit real multi-rank work cheaply.
Following @liopeer's guidance in #1982:
tests/ddp_helpers.py(new):USE_PYTEST_POOLflag (default off, folds in thegloo/non-Windows checks) and
setup_ddp. Adapted from and credited totorchmetrics'
conftest.py.tests/conftest.py:pytest_sessionstart/pytest_sessionfinishstart/stop thepool; registers the
DDPmarker. Nothing is enabled in CI.tests/loss/test_barlow_twins_loss.py: first consumer — two real 2-rank tests(
@pytest.mark.DDP+skipif(not USE_PYTEST_POOL)). The gradient test is aregression test for [BUG] BarlowTwinsLoss with
gather_distributed=Trueproduces gradient which is off by1/world_sizeunder DDP #1977 and fails on the rawdist.all_reduce.pyproject.toml:--import-mode=importlibinaddopts. Required because thepool uses
spawn, whose workers re-import the test module and deadlock underpytest's default
prependmode. It is a no-op for the rest of the suite.Three things worth your review, since you asked to iterate: the submission API
(
pytest.pool.starmap(worker, ...), workers return results to the parent toassert);
USE_PYTEST_POOLfolding the backend check into the flag rather than aseparate skip condition; and the Barlow tests using identical data per rank
(catches the
1/world_sizescaling bug but not heterogeneous-rank bugs).Tests
Two new tests in
tests/loss/test_barlow_twins_loss.pyrun on the pool; the restof the suite is unaffected (flag off → they skip). Verified on macOS/Apple Silicon,
Python 3.9, torch 2.8:
Before/after on the regression test (
USE_PYTEST_POOL=1, gradient test only):Full
make test-fastunder the new import mode:4 failed, 1687 passed— the 4 arethe known pre-existing macOS/Python-3.9 env failures (PyAV pickling, a py3.9
assertWarnsbug, the existing gloo DCL mismatch), unchanged by this PR and not hitby CI (Ubuntu 3.7/3.12).
Documentation
.rstfiles).Test-only change; no public API or
.rstdocs — no public functions added (thepool module, hooks, and
setup_ddpare internal; the worker helpers use inlinecomments per the surrounding test file's style).
Implications / comments / further issues
migrate the existing losses in
test_dist__gather__losses.pyonto the pool;add a heterogeneous-data-per-rank case; enable the pool in CI once it's proven
green on Linux; revisit
pytest.pool(global attr) vs a fixture.spawn/importlibmechanism isplatform-independent, but a maintainer sanity-check there would be valuable before
any CI-enablement PR. Happy to iterate.