perf: run each unit-test shard in parallel with pytest-xdist - #39026
Draft
AhtishamShahid wants to merge 28 commits into
Draft
perf: run each unit-test shard in parallel with pytest-xdist#39026AhtishamShahid wants to merge 28 commits into
AhtishamShahid wants to merge 28 commits into
Conversation
Every shard ran pytest single-process on a multi-core hosted runner, so ~75% of each runner sat idle. pytest-xdist[psutil] is already in the testing dependency group and pinned in the lockfile; CI just never passed -n. Measured on run 32843254784: PR wall clock 21.9m, of which 90-95% of every job is pytest itself (median queue wait 2s, setup ~1.1m/job). Simulating optimal re-sharding over the per-test timings from the master report-log artifacts shows 10 -> 30 shards moves the longest shard from 17.0m to 16.9m, so adding runners is not the lever -- the single largest directory, lms/djangoapps/discussion/rest_api/, is 16.9m on its own. - Add `-n logical --dist loadscope`. loadscope keeps each test class on a single worker so SharedModuleStoreTestCase's setUpClass work is still paid once per class rather than once per worker. - Skip the warnings-file write on xdist workers. pytest_sessionfinish fires on every worker as well as the controller; only the controller holds the aggregated report, and the workers would otherwise race for the next free filename and leave partial files for the compile-warnings-report job to double-count. - Log nproc before the run so the chosen worker count is visible. coverage is already configured with parallel/multiprocessing concurrency and pytest-cov combines worker data natively, so the coverage artifacts are unaffected.
|
Thanks for the pull request, @AhtishamShahid! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
The first xdist run failed collection on lms-4: Different tests were collected between gw1 and gw3 INSTRUCTOR_GET_ENDPOINTS and INSTRUCTOR_POST_ENDPOINTS are sets, and `@ddt.data(*THE_SET)` generates test names with positional indices (_01_, _02_, ...) assigned in iteration order. Set iteration order for str depends on PYTHONHASHSEED, which is randomized per process, so each xdist worker built a different index -> endpoint mapping and the workers disagreed on the node IDs. xdist requires identical collection on every worker, so it refused to run. Serially this was invisible: one process, one ordering, self-consistent. Sort at the two parameterization sites; the sets stay sets because the other eight uses are membership checks. Verified across five values of PYTHONHASHSEED that raw iteration order changes every time while the sorted order is identical. Also set fail-fast: false on the matrix -- one shard failing cancelled the other nine, which hides the rest of the failures for a full ~20 min cycle.
Run 2 surfaced the same collection mismatch in two more places. Both are the same underlying rule, from ddt.mk_test_name: when a parameter value is "trivial" (None/bool/int/float/str, or a list/tuple of those) ddt embeds str(value) in the generated test name. Non-trivial values get an index-only name instead. So any trivial value that varies per process gives each xdist worker a different node ID. Two ways that happened here: - common/djangoapps/student/tests/test_models.py (2 sites): parameterized over `set(CourseMode.ALL_MODES) - set(CourseMode.AUDIT_MODES)`. The elements are strs, so both the name and the positional index depend on set iteration order. Sorted. - lms/djangoapps/learner_home/test_serializers.py (2 sites): parameterized over tuples containing random_url(), which is evaluated once at import and differs per process. Replaced with literal URLs. Note the tuples that also contain uuid4() were already safe -- a UUID is not trivial, so those cases got index-only names; only the all-trivial tuples such as (random_url(), True, None, False) actually broke, which matches the observed failure at index _4. Checked the whole tree with an AST pass encoding ddt's real trivial/ non-trivial rules: 0 remaining sites. An earlier, looser version of that scan reported 183 hits, but dict literals and datetimes are non-trivial to ddt and therefore already index-named -- ddt's own docstring calls out the PYTHONHASHSEED dict-ordering hazard. pytest.mark.parametrize was scanned for the same two patterns and is clean.
…LIFO --dist loadscope split a module's test classes across workers, which broke the nesting that ModuleStoreIsolationMixin depends on. That mixin keeps process-global stacks -- __old_modulestores, __old_contentstores and __settings_overrides are mutable class attributes on the mixin, shared by every subclass -- layered on Django's own override_settings stack, and both SharedModuleStoreTestCase.setUpClass and ModuleStoreTestCase.setUp push onto them. Unwinding has to be strictly LIFO. Once it wasn't, settings was restored to the wrong object and every later start_modulestore_isolation raised: AttributeError: 'Settings' object has no attribute 'CONTENTSTORE' which accounted for 178 errors on shared-with-cms-1 and 82 on shared-with-lms-2, all cascading from one root cause. loadfile keeps every class in a module on one worker, preserving the within-file ordering the stacks rely on. Balancing is slightly coarser, which costs little here: the previous run showed ~2x on every shard against a 4-vCPU runner, so the ceiling is core count rather than distribution. This is a workaround, not a fix. The underlying problem is that the isolation stacks are process-global instead of per-class; making them per-class would remove the ordering constraint entirely.
ModuleStoreIsolationMixin and CacheIsolationMixin keep their bookkeeping in mutable class attributes -- __settings_overrides, __old_modulestores, __old_contentstores, __old_settings. Name mangling puts one list on the mixin and every subclass mutates it in place, so all ~443 test files that inherit ModuleStoreTestCase share five global stacks. That is only safe if every push is matched by a pop in strict LIFO order, and it isn't: - start_modulestore_isolation() enters an override_settings and only then calls clear_existing_modulestores() and modulestore(). If either raises, unittest skips tearDownClass, so the override is never exited. It stays on the global stack for the rest of the process. - override_settings.__exit__ restores settings._wrapped to the value that frame captured on entry. Popping a frame that is not the top therefore discards every override layered above it, which is why the symptom is "'Settings' object has no attribute 'CONTENTSTORE'" rather than a stack error -- settings had been rewound past the modulestore overrides. - setUpClassAndTestData() starts isolation and yields without a try, so an exception in the caller's setUpClass body leaks the same way. - end_cache_isolation() popped whenever the shared stack was non-empty, even if the entry at the top belonged to a different class. Serial execution happened to keep all of this balanced. That was a property of the fixed ordering, not of the design: under pytest-xdist each worker runs a different subset of classes and the nesting no longer holds. On shared-with-cms-1 a single leaked override cascaded into 178 errors. Changes: - Track isolation depth per exact class, read via cls.__dict__ so a subclass never sees or decrements a parent's count. end_*_isolation() is now a no-op when the class holds nothing, which also removes the IndexError the pact ProviderState views were catching. - Unwind the override if anything after override.__enter__() raises. - Wrap the setUpClassAndTestData() yield so a failing setUpClass body unwinds too. - Register addClassCleanup(end_modulestore_isolation) in SharedModuleStoreTestCase.setUpClass as a backstop: class cleanups run even when tearDownClass is skipped. The explicit tearDownClass call stays, so the ordering on the normal path is unchanged and the cleanup no-ops. - XModuleFactoryLock counts instead of holding a bool. Isolation nests, and an inner disable() previously switched factories off while an outer scope was still using them. Verified the semantics standalone against a fake override_settings: nested start/end, a raise after entry, double end(), end() without start(), and a subclass calling end() while the parent still holds isolation.
… the leaker
Run 5 showed the previous commit was only a partial fix: the cache cascade
went to zero, and shared-with-lms-2 dropped from 180 failures to 94, but the
modulestore leak survived in a different shape -- a bare AttributeError from
UserSettingsHolder rather than "'Settings' object has no attribute
CONTENTSTORE".
The gap is that end_modulestore_isolation() was not exception-safe:
drop_mongo_collections() # can raise
XMODULE_FACTORY_LOCK.disable()
cls.__settings_overrides.pop().__exit__(None, None, None) # then never runs
Django's override_settings.disable() ends with `del self.wrapped`, so an
override object is single-use: a frame skipped here can never be unwound
later. drop_mongo_collections() talking to a Mongo shared by four xdist
workers is exactly the kind of step that fails intermittently, and one such
teardown poisons every later test in that worker. Unwinding now happens in
nested finally blocks, so the override, the cache isolation and the signals
are always restored.
Also adds a diagnostic. The test that trips over a corrupted settings stack
is almost never the one that corrupted it -- run 5 reported 117 identical
failures with no indication of the origin. start_modulestore_isolation() now
checks that settings.CONTENTSTORE is readable before touching anything and,
if it is not, raises immediately naming the classes that still hold an
isolation. That turns a cascade of downstream victims into one message
pointing at the leak, both for this investigation and for future
regressions.
The leak detector added in the previous commit named the culprit directly:
494 of the failures on shared-with-lms-2 reported "Isolations still open:
['BaseCoursewareTests']".
BaseCoursewareTests.tearDownClass had its two steps in the wrong order:
super().tearDownClass() # ends isolation, drops mongo
cls.store.delete_course(cls.course.id, cls.user.id) # then uses the modulestore
super().tearDownClass() runs end_modulestore_isolation(), which calls
drop_mongo_collections() and restores the settings override. The
delete_course() that follows therefore runs against a modulestore that no
longer holds the course, raises, and aborts the remainder of tearDownClass.
Swapping the two lines is the whole fix: clean up the course while the
isolation that created it is still in place.
An AST scan over the tree for tearDownClass bodies that touch the modulestore
after super().tearDownClass() finds only this one real instance. The two
other hits are the SplitModulestoreCourseIndex deletes in django_utils.py,
which are documented as deliberately ordered that way.
7388655 used `git add -A` and swept in four untracked repro/scratch files that have nothing to do with this branch: common/djangoapps/third_party_auth/tests/test_repro_hq12633.py mitxodl_backends.py openedx/core/djangoapps/contentserver/test/test_repro_hq12620.py openedx/core/djangoapps/notifications/tests/test_h1_validation.py Untracked with `git rm --cached`, so they are gone from the branch and the net diff against master no longer contains them, while the working copies stay on disk where they were. Also fixes the isolation leak detector, which was reporting the wrong class. The register popped its last entry regardless of who owned it. Class-scoped and test-scoped isolation nest, so when a class leaked an override the next class to finish would pop the leaker's entry, stay consistent with the override stack, and report nothing -- while the register quietly drifted and started naming innocent classes. That is why run 6 and run 7 both blamed BaseCoursewareTests, which defines no test methods and therefore never has setUpClass run at all. The register now records (class name, id(override)) and end_modulestore_ isolation() checks that the innermost open isolation actually belongs to the class that is ending. If it does not, it raises immediately naming both the class unwinding and the class that leaked, rather than letting the unwind restore settings._wrapped to a stale object and produce a few hundred unattributable AttributeErrors downstream. Checked against a standalone model of the two stacks: an out-of-order unwind is caught, correct nesting passes, and a single class holding two nested isolations -- class-level plus per-test, which is legitimate -- does not trip it. An earlier version of this check compared id(override) instead of the owner and detected nothing, because both stacks pop in lockstep and stay mutually consistent even when the owner is wrong.
…t caused it Run 8 localised the remaining failures precisely: 123 of 124 on shared-with-lms-2 were on gw0, and all 368 on shared-with-cms-1 were on gw1. One worker per shard gets poisoned and every later test in that process fails; the other workers are clean. The detector also ruled out the cause I had assumed -- it reports "Isolations still open: (none)" and the ownership check never fired once, so nothing leaks a settings override and unwinding is always correctly ordered. The base Settings object simply loses CONTENTSTORE partway through the run, even though lms/envs/test.py assigns it unconditionally. Every `del settings.X` in the tree (11 sites) is correctly wrapped in @override_settings(), so that documented footgun is not it either. Two changes. Stop depending on the setting being present. start_modulestore_isolation() read settings.CONTENTSTORE only to snapshot it for an assertion at teardown, and the isolation overrides CONTENTSTORE regardless, so the snapshot is bookkeeping rather than a requirement. It now uses getattr(..., None) and asserts only on what it actually captured. Nearly all the affected tests -- logistration, bookmarks, cohorts, enrollments, gating -- never touch the contentstore and fail purely because they inherit the mixin. This is mitigation, not a root-cause fix, so it reports rather than hides: the first isolation to see the setting missing emits a RuntimeWarning carrying the full attribution instead of raising. Raising is what turned one upstream bug into several hundred downstream errors and made the origin unfindable. Attribute the corruption. end_modulestore_isolation() now checks whether CONTENTSTORE is still readable once it has unwound. Every isolation passes through there, so the first teardown that sees it missing captures PYTEST_CURRENT_TEST -- naming the test that was running when it went. Alongside that it records the identity of the base Settings object, its SETTINGS_MODULE, the number of override layers, and whether CONTENTSTORE is in the base object's vars. Those distinguish the two possible mechanisms, which need different fixes: the base Settings was replaced, or the attribute was deleted off the base object still in place.
…tic]
Run 9 narrowed the last failures to a single anomaly and the tolerance fix
turned most of their damage off: shared-with-lms-2 went from 124 failures to
3760 passed, shared-with-cms-1 from 368 failures to 2 failures and 9 errors,
8/10 shards green.
What the attribution recorded, consistently:
base Settings id=0x7f9eed0fc770 identical in every report
module=lms.envs.test correct module
CONTENTSTORE in base vars=False attribute absent from the base object
Identity unchanged rules out a settings reload or a replaced Settings object;
absent from the base object's own __dict__ rules out an override_settings
frame, which would shadow rather than remove. So something deletes the
attribute off the base object, and only transiently -- 16 isolations out of
3760 tests saw it missing, and the rest ran fine.
That also corrects the earlier reading of run 8. This never permanently
poisoned a worker; the previous version of this check raised, so the first
occurrence failed a test, broke its teardown and cascaded. The anomaly is
narrow and the error handling was manufacturing the rest.
No candidate survives static search: all 11 `del settings.X` sites in the tree
are correctly wrapped in @override_settings(), no code deletes CONTENTSTORE by
name, nothing touches settings.__dict__, and lms/envs/test.py assigns it
unconditionally at module level. So catch it in the act instead: wrap
__delattr__ on both Settings and UserSettingsHolder and dump a stack trace plus
PYTEST_CURRENT_TEST when CONTENTSTORE is the name being removed.
Diagnostic only, to be removed once the cause is fixed. Verified locally that
it fires for both classes, stays silent for other names, and leaves deletion
behaviour unchanged.
…diagnostic] The deletion tracer from the previous commit fired zero times in run 10 while 41 corruption reports were logged in the same shard, so CONTENTSTORE is never deleted via __delattr__ on either Settings or UserSettingsHolder. That rules out the last mechanism I had a candidate for. The same reports showed something I had not been recording: the override layer count climbs -- 1, then 10, 11, 12 ... 18 -- so override_settings frames are accumulating from a source that is not modulestore isolation (the isolation register consistently reports none open). A leak of that shape can mask a setting without deleting it anywhere. The old diagnostic only inspected the bottom of the chain, which cannot tell those cases apart. It now walks every layer and reports, per layer, the type, whether CONTENTSTORE is in that layer's own vars, and whether it is in that layer's _deleted set -- the two ways a lookup can be blocked partway down. It also reports MODULESTORE in the base object as a control: if both settings are absent from the base then the walk is not reaching the real Settings and the earlier "CONTENTSTORE in base vars=False" readings were misleading; if only CONTENTSTORE is absent, the anomaly is genuinely per-attribute. Validated the walk offline against real Django objects, including a holder with CONTENTSTORE in _deleted, which reproduces the bare AttributeError seen in earlier runs and is distinguishable from the messaged one.
… diagnostic]
The per-layer chain dump found the actual mechanism, and it is not what the
earlier diagnostic reported. Every sample in run 11 says:
MODULESTORE in base vars=True CONTENTSTORE in base vars=True
UserSettingsHolder <- UserSettingsHolder(DELETED) <- UserSettingsHolder
<- UserSettingsHolder <- Settings(has, module=lms.envs.test)
The base Settings has CONTENTSTORE the whole time. A UserSettingsHolder partway
up the chain carries CONTENTSTORE in its _deleted set, and UserSettingsHolder
.__getattr__ raises a bare AttributeError for anything in _deleted before it
delegates downwards. So the setting is masked, never deleted, which is why the
__delattr__ tracer correctly reported nothing.
That also retracts the previous commit message: "attribute absent from the base
object" was an artefact of the old diagnostic, which walked the chain with
hasattr(wrapped, 'default_settings') and did not stop where it claimed. The base
was intact in every run.
The frame also leaks -- layer counts climb 10, 11, 12 ... 18 within a shard -- so
once such a holder appears it stays in the chain and masks the setting for
everything that follows. Deleting a setting inside @override_settings() is the
documented way to test its absence, so the bug is the frame not exiting, not the
delete.
Two additions to find which call site it is: the tracer now announces itself at
install time, which distinguishes "nothing deleted" from "tracer never ran"; and
each layer in the dump now reports the full contents of its _deleted set plus
the settings it overrides, which should name the override_settings call that
created it.
…y diagnostic]
Run 12 confirmed the tracer really is installed -- it now prints a marker, and
five appear per shard, one per xdist worker. So it genuinely observes zero
CONTENTSTORE deletions while the chain dump shows a holder carrying CONTENTSTORE
in its _deleted set. Django's UserSettingsHolder starts with an empty _deleted
and only __delattr__ ever adds to it, so those two facts cannot both hold in one
process, and reconciling them by reading source has not worked.
Reproduced the exact symptom locally instead:
o = override_settings(); o.enable(); del settings.CONTENTSTORE
-> _deleted == {'CONTENTSTORE'}
-> settings.CONTENTSTORE raises, while the base Settings still has it
That is the CI symptom precisely, including the base staying intact, and it
comes from a frame that is entered and never exited. Deleting a setting inside
@override_settings() is Django's documented way to test a setting's absence, so
the delete is fine; the leak is the bug.
What is missing is which call site made the frame, and a holder records nothing
about its origin. UserSettingsHolder.__init__ is now wrapped to store the first
non-Django frame that created it, and the chain dump prints that for the masking
layer. Uses sys._getframe rather than traceback.extract_stack because it runs on
every override in the suite.
No reload of django.conf exists anywhere in the tree, so an un-patched tracer is
ruled out as the explanation; the creation site should settle it.
…agnostic]
Run 13 showed the two failing shards are not the same failure, which the
earlier single-layer diagnostic could not distinguish:
shared-with-lms-2 base Settings HAS CONTENTSTORE, a UserSettingsHolder above
it carries CONTENTSTORE in _deleted and masks it
shared-with-cms-1 base Settings genuinely lacks CONTENTSTORE while having
MODULESTORE, 106 samples, module=cms.envs.test
The second one cannot be explained by anything found so far. cms/envs/test.py
assigns CONTENTSTORE unconditionally at module level on line 73, exactly once,
and Django's Settings.__init__ copies every uppercase module global. The
__delattr__ tracer is confirmed installed, five markers per shard, and still
reports zero deletions.
That leaves one untested possibility: the attribute never survives Django setup
in the first place, so no test is involved and nothing is ever deleted. The
tracer now reports, at install time and before any test has run, whether each
watched setting is readable, along with the resolved settings module. Present
means a test removes it later and the existing wrappers will catch that; missing
means the search moves to settings loading and away from the test suite
entirely.
Also worth noting for whoever picks this up: the 9 remaining errors in that
shard are all SandboxServiceTest, a plain TestCase whose setUpClass calls
contentstore(), which reads settings.CONTENTSTORE directly. It is a victim of
the anomaly rather than a cause.
… fails Run 14 brought back InvalidCacheBackendError and KeyError: 'course_index_cache' on shared-with-lms-1, 38 of them. That is the error the cache-isolation depth guard drove to zero in run 5, so its return points at a defect introduced by these commits rather than at the pre-existing behaviour. start_modulestore_isolation() takes cache isolation first and only then builds and enters the settings override. end_modulestore_isolation() keys off the modulestore depth and returns immediately when it is zero, without unwinding the cache. So anything raising between those two points strands a CACHES override for the remainder of the process, and it surfaces much later as a missing named cache rather than as an error at the point of failure. cls.MODULESTORE() and cls.CONTENTSTORE() are arbitrary callables and copy.deepcopy is not total, so the window is real. The obvious repair -- unwinding cache isolation on that early return -- is wrong, and worth recording so it is not tried again. end_modulestore_isolation() is called twice on the normal path, once from tearDownClass and once from the addClassCleanup backstop, and on the second call the modulestore depth is already zero while CacheIsolationTestCase still legitimately holds cache isolation of its own. Unwinding there would pop a frame that class has not finished with, and its own tearDownClass would then pop a second time. Fixed at the source instead: the region between taking cache isolation and recording the modulestore isolation now unwinds the cache isolation and restores the signals if it raises. Checked against a model of the depth bookkeeping for the three real sequences -- SharedModuleStoreTestCase including the duplicate teardown call, the per-test ModuleStoreTestCase path, and a failure inside start_ -- all balance, and the same model reproduces the leak with the fix removed.
Strips the instrumentation added across runs 10-14 and keeps the fixes it was
built to find.
Removed: the __delattr__ tracer on Settings and UserSettingsHolder, the
UserSettingsHolder.__init__ creation-site capture, the settings-chain dump and
its corruption report, the open-isolation register and its ownership check, and
the install-time presence probe in cms/conftest.py. That probe was also flawed
-- it read the setting through LazySettings.__getattr__, which caches the value
it reads, so it perturbed the state it was measuring.
Kept, because each is a real defect the runs demonstrated:
django_utils.py per-class isolation depth so end_*_isolation() is
idempotent; unwind in finally so a failed teardown cannot
strand a single-use override_settings frame; unwind cache
isolation if start_ fails after taking it; addClassCleanup
backstop for a setUpClass that raises; try/except around
setUpClassAndTestData's yield; tolerate a missing
CONTENTSTORE and warn instead of raising
djangolib/utils.py only pop cache overrides this class actually pushed
factories.py XModuleFactoryLock counts instead of holding a bool
pytest_hooks.py skip the warnings-file write on xdist workers
Also untracks the four local scratch files that 7388655 swept in with
`git add -A` and that later `git add -A` calls re-added after the first removal,
and adds .gitignore entries so it cannot happen a third time. They stay in the
working tree, untracked.
Check Django Migrations is now the longest workflow on a pull request at 12.2 minutes, ahead of unit-tests at 11.9, so it sets the wall clock and no further unit-test work reduces time-to-green until it is addressed. Almost all of it is one step: applying ~1811 migrations (1087 in-repo, 724 from dependencies) serially from an empty database, twice, once for LMS and once for CMS. That is 9.4 of the 12.2 minutes. Of the last 200 commits on master, 2 touch a migration file. The other 198 runs re-derive a schema identical to the previous one. So gate the expensive job on whether the diff actually touches anything that can change the schema. models.py is included deliberately: a model changed without a matching migration is one of the things this workflow exists to catch. Only pull_request is gated. push, merge_group and workflow_dispatch still run the full check unconditionally, which matters because a RunPython migration that imports application code can be broken by a change that touches no migration or model file. That case is rare, and master still catches it, but it is a real narrowing of per-PR coverage rather than a free win. The detection is a git diff against the merge base rather than a third-party action, to avoid taking a new dependency in a required check. Verified against history: a real migration commit matches, this branch's own diff does not, and tests/test_models.py correctly does not match while models.py does. allowed-skips keeps the required "Migrations checks successful" job green when the check is skipped, without weakening it when the check actually fails.
Quality checks has been failing since 7388655, the second commit on this branch. It passes on master, so this was caused here and is not pre-existing -- I said otherwise earlier and was wrong; the run history disproves it. Three violations, all introduced by this branch: - I001 in learner_home/test_serializers.py: one blank line too many between the import block and the TEST_URL constants added for the ddt fix. - B018 in django_utils.py: `settings.CONTENTSTORE` as a bare statement reads as a useless expression. The pylint pragma on it does not apply to ruff. Replaced with getattr, which expresses the same probe as a real call -- the point is whether the lookup raises, not what it returns, because UserSettingsHolder raises AttributeError for a setting in its _deleted set. - B028 in django_utils.py: warnings.warn without an explicit stacklevel, so the warning was attributed to the mixin rather than to the caller that triggered it. stacklevel=2 makes it point at the test. ruff check now passes on every file this branch touches.
The previous commit fixed ruff B018 ("useless expression") by rewriting the
bare `settings.CONTENTSTORE` access as getattr, which then tripped pylint
C7630 literal-used-as-attribute from edx-lint. Quality checks went green and
Pylint Checks went red on the same commit -- one linter traded for another.
Keep the bare attribute access, which is the honest expression of the probe,
and tell both linters explicitly. The access has to happen for its side effect:
UserSettingsHolder.__getattr__ raises AttributeError for a setting in its
_deleted set, so observing whether the lookup raises is the whole point.
Note the three-argument getattr calls elsewhere in this file, used to tolerate a
missing MODULESTORE/CONTENTSTORE when snapshotting, were not flagged. C7630
targets the two-argument form only, since getattr with a default cannot be
rewritten as attribute access.
ruff check passes on every file this branch touches.
Two changes to the ~9 minute migrate step, for the pull requests that do touch a migration and so still pay it in full after the previous gating commit. Cache the migrated schema. Applying every migration from an empty database produces an almost identical result on every run, so dump it -- including the django_migrations table, which records what has already been applied -- and reload it instead. restore-keys matters more than the exact key: on a pull request that adds one migration the exact key misses, an older schema is restored, and migrate applies only the delta. LMS and CMS point at the same database, so a single dump covers both. A restored schema can be unusable: a migration squashed, renamed or removed since the dump was taken leaves django_migrations describing a graph that no longer exists. The step falls back to dropping the database and migrating cold rather than reporting a failure that is really a stale cache, which would be indistinguishable from a genuine broken migration. Tune MySQL. innodb-flush-log-at-trx-commit=0, innodb-doublewrite=0, sync-binlog=0, skip-log-bin and performance-schema=OFF, with the datadir on a tmpfs. All of it is safe here -- the database is discarded when the job ends, so there is nothing to lose on an unclean shutdown, and writes never reach a disk. Reported at roughly 2-3x on DDL-heavy workloads, which is what applying ~1800 migrations is. This required starting MySQL by hand: a `services:` block can pass docker create options but not server flags. Nothing referenced job.services.mysql.id, only the client over the published port, so the switch is contained. The root password is now fixed rather than random so the fallback above can drop and recreate the database. Squashing migrations, the remaining structural option, is deliberately not attempted: Trail of Bits measured 13% for PyPI and rejected it on maintenance grounds, and with 1087 in-repo migrations across 86 apps the coordination cost here is far higher for a similar return.
Measured both tiers separately on workflow_dispatch runs rather than shipping them together and assuming the total was the sum of the estimates. They did not behave the same way. Schema cache (kept). Run Tests 630s cold -> 41s warm. The cached dump restores in ~25s and both migrate invocations then report "No migrations to apply", with zero migrations re-applied. 9.4 min -> 41s for the step. MySQL tuning (reverted). tmpfs plus innodb-flush-log-at-trx-commit=0, innodb-doublewrite=0, sync-binlog=0 and skip-log-bin produced no improvement at all: the LMS migrate alone took 9m52s against a 9.4 min baseline for LMS and CMS together. The cold-run timeline shows why -- 1297 migrations in ~590s, about 0.45s each, spent in Django rebuilding project state per migration rather than in the database. The 2-3x figure I applied comes from OLTP benchmarks; this is DDL, and the workload does not match. Reverted rather than carried, since it also required hand-starting the container with a manual health-wait. The same timeline corrects another assumption: splitting LMS and CMS into a matrix would have been worthless. CMS takes 18 seconds, because both point at the same database and LMS has already applied everything. It was never going to halve anything. The service block returns, with a fixed root password rather than a random one so the cache fallback can still drop and recreate the database, and the docker exec calls now address the container by job.services.mysql.id.
The five remaining unit-test failures were not five problems. Every one of
them, across two shards, came from the same place:
ERROR edx.student:login.py:691 {'success': False,
'value': Markup('Too many failed login attempts. Try again later.')}
which is login.py raising on `getattr(request, "limited", False)`. That flag is
set by django_ratelimit, which counts attempts in the Django cache. The cache is
process-global and, unlike the database, is not rolled back between tests, so
counts left by earlier tests in the same worker make a later login return 400 or
429 where the test expects 200.
Serial ordering hid it: these tests happened to run before enough attempts had
accumulated. Under pytest-xdist a different mix of tests shares each worker, so
the counter is in a different state by the time they run. Nothing about the
tests themselves changed.
test_reset_password.py in the same package already calls cache.clear() for
exactly this reason, so this follows the existing convention rather than
inventing one: LoginSessionViewTest, LogoutTests, PostLoginRedirectFiltersTest
and TestActivateAccount now clear the cache in setUp.
Worth noting these tests are order-dependent on master too. The failure is
latent there rather than absent -- adding a login test ahead of them in the
serial order would surface it just as well.
Three tests in RecoverAccountTests failed with `assert 0 == 1` on
len(mail.outbox). The captured log shows the email was attempted and lost:
ERROR ...recover_account:99 Unable to send email to amy@newemail.com
...
File openedx/core/djangoapps/ace_common/templatetags/ace.py:69
return request.site, request.user, message
AttributeError: 'WSGIRequest' object has no attribute 'site'
The command sends an ACE message, and the ace template tags resolve the
"current" request through crum's thread-local, which nothing resets between
tests. emulate_http_request() sets .site on whichever request it finds there,
so success depends on what an earlier test left behind. When that is a request
without .site the tags raise, the command catches it as "Unable to send email",
and mail.outbox stays empty -- so the assertion fails several frames away from
the actual cause.
The tag also prefers a request already in the template context over crum's, so
there is more than one object in play and decorating one is not sufficient.
Install a well-formed request in setUp rather than inheriting one, and clear it
afterwards via addCleanup so this class does not leak into the next test the way
it was leaked into.
This is the same shape as the login rate-limit failures fixed in the previous
commit: process-global state that the database rollback does not touch, which a
fixed serial order happened to leave in a workable state. Latent on master, not
introduced here.
The last remaining unit-test failure:
test_create_content_gating_partition_disabled
assert partition is None
AssertionError: assert ContentTypeGatingPartition(id=51, ...)
The test creates ContentTypeGatingConfig(enabled=False) and expects no partition
to be built. ContentTypeGatingConfig is a ConfigurationModel, and those cache
current() in the Django cache, so the newly written row was ignored in favour of
a value an earlier test had cached with enabled=True.
The class extends CacheIsolationTestCase but its setUp never called
super().setUp(), with a pragma silencing the warning about it. That method is
exactly what calls clear_caches() and registers the cleanup that clears them
again, so skipping it opted the class out of the isolation its base class exists
to provide. Restoring the super() call and dropping the pragma is the whole fix.
Same family as the previous two commits -- process-global state untouched by the
database rollback, which serial ordering happened to leave workable. Here the
opt-out was explicit in the source rather than incidental.
The cleanup added in the previous commit set crum's current request to None unconditionally, which is a behaviour change for whatever runs next in the same worker rather than a neutral restore. Save and put back what was there. The class still stops depending on leftover state, which was the point; it just no longer changes that state for everyone else on the way out.
shared-with-cms-1 fails intermittently with a large cascade -- 29 failures and 21 errors in the latest instance -- whose counts tell the story: 155 TransactionManagementError, 41 "NOT NULL constraint failed: user_api_userpreference.user_id", and 9 "'Settings' object has no attribute CONTENTSTORE". Following the first traceback leads to test_sandboxing.py:66. SandboxServiceTest and SandboxServiceForLibrariesV2Test build a SandboxService around contentstore() in setUpClass, and SandboxServiceTest uploads an asset with upload_file_to_course -- a helper that lives in xmodule.modulestore.tests.django_utils. Both extended plain TestCase, so neither got any contentstore isolation; they simply read whatever settings.CONTENTSTORE happened to hold. When an earlier test has left an override_settings frame masking that setting, the upload raises inside setUpClass with the class-level atomic already open, and every later test in that worker fails on the broken transaction. That is where the userpreference IntegrityError comes from -- not from anything to do with user preferences. Both classes now use SharedModuleStoreTestCase, which is class-scoped like their setUpClass and establishes the CONTENTSTORE override they were relying on finding by luck. This does not fix the underlying masking bug, which is still open and documented in the PR description. It removes this cascade's entry point, which is what makes the bug expensive: nine errors turning into fifty.
Restoring super().setUp() in the previous commit fixed test_create_content_gating_partition_disabled and immediately exposed test_create_content_gating_partition_partition_id_used, which failed with "Expected 'warning' to have been called". enabled_for_course() opens with `if not correct_modes_for_fbe(course_key): return False`, so a course needs audit and verified modes before the config's enabled=True has any effect. That test never created them, so create_content_gating_partition() returned at its first branch and never reached the LOG.warning the test asserts on. It passed only because test_create_content_gating_partition_happy_path -- which does create the modes -- had polluted the ConfigurationModel cache. Removing the pollution removed the accident holding it up. test_create_content_gating_partition_no_scheme_installed had the identical gap. It was not failing, because it asserts None and got None either way, but it was returning at the first branch rather than exercising the UserPartitionError path it patches. Given the same one-line cause, fixed here too rather than left as a test that passes without testing anything.
shared-with-lms-1 fails intermittently with a large InvalidCacheBackendError
cascade -- 114 occurrences in the latest instance, 0 in each of the three runs
before it, on identical code. The oscillation is the tell: this is not a test
being wrong, it is cleanup code turning a transient condition into a failure.
CacheIsolationMixin.clear_caches() loops over settings.CACHES and resolves each
alias through Django's connection handler:
for cache in settings.CACHES:
caches[cache].clear()
Those two can disagree. ModuleStoreIsolationMixin's isolation adds aliases --
course_index_cache, loc_cache, mongo_metadata_inheritance -- via
override_settings, and leaving that frame fires setting_changed for CACHES,
which resets the handler. While frames are unwinding, settings.CACHES can still
list an alias the handler no longer knows, and the lookup raises. The same
window produces the asgiref _CVar AttributeError seen alongside it.
An alias that cannot be resolved has no cache to clear, so there is nothing for
this method to do about it. Skip it. Raising achieves nothing except failing the
test -- and because clear_caches() runs from setUp and from an addCleanup, one
transient disagreement takes out every test in the class and then the rest of
the worker.
This does not fix the underlying override_settings frame problem, which is still
open. It stops that problem from being amplified by the cleanup path, which is
where nearly all of its visible damage comes from.
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
2 participants
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.
Runs each unit-test shard across all the runner's cores instead of one.
Draft. The speedup is confirmed and reproducible. The remaining failures are pre-existing order-dependent tests, listed below.
Result
Runner is
ubuntu-24.04,nproc: 4(2 physical), so ~2× is the ceiling and we're at it. Both wall clock and cost drop.Adding shards can't do this: packing the real per-directory timings (from the
--report-logartifacts this workflow already uploads) shows 10 → 30 shards moves the longest shard 17.0 m → 16.9 m and triples the bill, becauselms/djangoapps/discussion/rest_api/alone is 16.9 m and a directory can't be split.Why each file changed
Only the first file is the feature. Everything else is a pre-existing bug that a fixed serial order happened to hide — all of it passes on master today.
.github/workflows/unit-tests.yml-n logical --dist loadfile.loadfilerather thanloadscopebecause splitting a module's classes across workers breaks test isolation. Plusfail-fast: false, so one bad shard stops hiding the other nine.openedx/core/pytest_hooks.pypytest_sessionfinishfires on every worker, not just the controller. Without a guard the workers race for the next freepytest_warnings_N.jsonname and leave partial files forcompile-warnings-reportto double-count.lms/djangoapps/instructor/tests/test_api.py@ddt.data(*A_SET)— ddt embeds trivial values in the generated test name, and set iteration order followsPYTHONHASHSEED, so each worker built different node IDs and xdist refused to run. Sorted.common/djangoapps/student/tests/test_models.pyset(ALL_MODES) - set(AUDIT_MODES). Sorted.lms/djangoapps/learner_home/test_serializers.py@ddt.data((True, random_url()))evaluated at import, so the URL differed per worker. Literal URLs instead.openedx/core/djangolib/testing/utils.pyend_cache_isolation()popped the shared override stack whenever it was non-empty, even when the top belonged to another class. Now only unwinds what this class pushed.xmodule/modulestore/tests/django_utils.pyxmodule/modulestore/tests/factories.pyXModuleFactoryLockheld a bool, but isolation nests, so an innerdisable()switched factories off while an outer scope still needed them. Now a refcount.openedx/core/djangoapps/courseware_api/tests/test_views.pytearDownClasscalleddelete_course()aftersuper().tearDownClass()had dropped the collections, so it raised and aborted the rest of teardown. Two lines swapped..gitignoregit add -A.django_utils.pyin detailend_*_isolation()is safe to call twice — fromtearDownClassand from a cleanup — and does nothing when the class holds none. Also removes theIndexErrorthe pactProviderStateviews were catching.finally. Django'soverride_settings.disable()ends withdel self.wrapped, so an override is single-use: a frame skipped during a failed teardown can never be unwound.drop_mongo_collections()talking to a Mongo shared by four workers fails often enough for this to matter.start_modulestore_isolation()takes cache isolation before the modulestore override, andend_returns early when the modulestore depth is zero — so a failure between the two leaked aCACHESoverride, surfacing later asInvalidCacheBackendError. Fixed at the source; unwinding on the early-return path would double-popCacheIsolationTestCase's own frame.addClassCleanupbackstop.tearDownClassis skipped entirely when a subclass'ssetUpClassraises; a class cleanup still runs. The explicit call stays, so normal-path ordering is unchanged.CONTENTSTOREand warn. The old value was only kept to assert against at teardown and the isolation overrides it anyway. Raising there turned one upstream problem into ~480 downstream failures and hid the origin.Also in this PR: the migration check
Check Django Migrationsbecame the longest workflow on a PR (12.2 min) once unit-tests dropped to ~12, so it set the wall clock. Three things were tried and measured separately, which turned out to matter — they did not behave alike.Why the gate is safe. Only 2 of the last 200 commits on master touch a migration.
models.pyis included in the filter, because a model changed without a migration is exactly what this catches.push/merge_group/workflow_dispatchstay ungated — aRunPythonmigration that imports application code can break from a code-only change, and master still catches that.allowed-skipskeeps the required check green.Why the MySQL tuning failed. The cold-run timeline shows 1297 migrations in ~590s, ~0.45s each, spent in Django rebuilding project state per migration — not in the database. The "2-3x from tmpfs" figure is from OLTP benchmarks; this is DDL, and the workload does not match.
That same timeline kills another idea worth recording: splitting LMS and CMS into a matrix would have been worthless. CMS takes 18 seconds, because both point at the same database and LMS has already applied everything.
Still failing
Down to a handful, and the login cluster turned out to be one cause rather than several.
Every one of
LoginSessionViewTest,LogoutTests,PostLoginRedirectFiltersTestandTestActivateAccountfailed withassert 400 == 200orassert 429 == 200, and the captured log says the same thing each time:That is
login.pyraising ongetattr(request, "limited", False), set by django_ratelimit, which counts attempts in the Django cache. The cache is process-global and, unlike the database, is not rolled back between tests — so counts left by earlier tests in the same worker break later ones. Serial ordering hid it; a different mix of tests per worker exposes it. Fixed withcache.clear()insetUp, followingtest_reset_password.pyin the same package. These are order-dependent on master too; the failure is latent there, not absent.Globally disabling rate limiting in tests would be the wrong fix — several tests deliberately assert rate-limit behaviour.
Remaining, all order-dependent and rotating between runs:
assertNumQueries(test_model_data.py,test_batch_generate_id.py) — the missing query is the transactionBEGIN/SAVEPOINT, not re-issued when the connection is already openTestContentTypeGatingPartitionpartition-id assertionSandboxServiceTest— a plainTestCasewhosesetUpClasscallscontentstore(), so it readssettings.CONTENTSTOREoutside the isolation mixinThe open question
A
UserSettingsHoldersometimes carriesCONTENTSTOREin its_deletedset and never exits, masking a value the baseSettingsstill holds. Deleting a setting inside@override_settings()is Django's documented way to test absence, so the delete is fine — the leak is the bug. It fires ~11-50 times per shard and now causes warnings rather than failures.Investigation notes for whoever picks this up: a
__delattr__tracer on bothSettingsandUserSettingsHolderrecorded zero deletions while the flag was present, and nodel settings.Xin the tree is unguarded. An install-time probe was inconclusive because reading throughLazySettings.__getattr__caches the value and perturbs the state. The clean next experiment is to captureid()of the baseSettingsat setup and compare it at failure, which separates "same object mutated" from "different object" without that side effect.Follow-ups
cms-1holds a whole runner for 213 tests.--covtopush: masteronly. (COVERAGE_CORE=sysmonisn't available — Python 3.12 refuses it whenbranch = true.)unit-test-shards.jsonwithpytest-split, removing the directory-granularity floor and thecollect-and-verifyjob.SharedModuleStoreTestCase. Intest_views_v2.py, 178 tests each take ~3.0 s of pure fixture cost — the only fix that removes work rather than redistributing it, and it speeds up local runs too.