test: schedule functional tests by class (--dist loadscope) instead of by xdist_group mark - #835
Merged
Merged
Conversation
#833 set `--dist loadgroup` and marked one class. #834 asked whether to mark the rest by hand or group every class automatically with `--dist loadscope`, and said deciding needed a timing comparison of a full run under each. Here it is, on the functional suite at `-n auto` (12 workers, 406 tests, 230 classes), counting setups of dbt's class-scoped `project` fixture - one per schema created and dbt project parsed - over two rounds each: mode setups wall --dist load (the default) 252 / 255 339s / 351s --dist loadgroup + 7 marks 290 / 287 361s / 363s --dist worksteal 233 / 233 345s / 359s --dist loadscope 230 / 230 314s / 308s `loadscope` is the only one that reaches one setup per class, and it is also the fastest. The tail-latency tradeoff the issue anticipated - one long class pinning one worker and stretching the tail - does not show up at this suite's shape: 230 work units over 12 workers leaves plenty to balance with, and the redundant setups cost more than the coarser scheduling saves. The surprise is `loadgroup`, which is worse than doing nothing. The issue assumed it "schedules unmarked tests exactly as `load` does"; it does not. `LoadGroupScheduling` is `LoadScopeScheduling` with an unmarked test's scope set to its own nodeid, so every unmarked test becomes its own work unit, handed out one at a time from a queue. Plain `load` instead sends batches of consecutive tests, deliberately - xdist's own comment there says it is preserving the collection order that minimizes "the number of necessary fixture setup/teardown". Marking seven classes saved 7 duplicate setups and gave up about 40 elsewhere. Hand-marking would not have converged anyway. The classes that actually duplicate are not the ones the issue's AST scan predicted: the worst under `load` were TestQueryOptionsCore (5 copies) and TestDmlRefresh (4), neither of which has a class-scoped fixture calling `run_dbt`, so the scan cannot see them. They are simply classes with many tests, and every copy still pays for a schema and a dbt parse. Which classes split also moves run to run with worker timing. `loadscope` needs no list and cannot go stale. The `xdist_group` mark on TestOpenquery goes with it: under `loadscope` a class is a work unit either way, so the mark is inert, and leaving it would leave the docstring crediting a mechanism that is no longer doing the work. Every mode reported the same results - 356 passed, 48 skipped, 2 xfailed - so this changes scheduling only. Re-run on this tree: 230 setups, no class set up twice. The unit suite is unaffected: 500 passed, 4.4s under `load`, 3.7s here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestCatalogAcrossDatabases and TestCrossDB each created a database named `secondary_db`, and each dropped it when done - with `SET SINGLE_USER WITH ROLLBACK IMMEDIATE`, which boots whatever sessions are still on it. A database is instance-wide, so this is #833's linked-server hazard in another shape: two holders of one name, and whoever finishes first pulls it out from under the other. They live in different modules, so no `--dist` mode keeps them apart. `loadscope` groups within a class; `loadgroup` could put both classes in one group, but the preceding commit measured that scheduler as the wrong default for the suite as a whole. Distinct names fix it without depending on scheduling at all. Found while auditing class-scoped fixtures for #834. It has not been seen failing - each class holds a single test, so the overlap window is one test long - which is exactly why it is worth closing before it is. Co-Authored-By: Claude Opus 5 (1M context) <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.
Closes #834. Follows #831, which set
--dist loadgroupand marked one class.#834 asked whether to mark the remaining expensive classes with
xdist_groupby hand or switch to--dist loadscopeand group every class automatically, and said the answer needed a timing comparison of a full-n autorun under each. This is that comparison, and its answer:loadscope.The measurement
Functional suite at
-n auto(12 workers, 406 tests, 230 classes) against a local 2022 instance, two rounds per mode. The setup count comes from a temporary plugin hookingpytest_fixture_setupfor dbt's class-scopedprojectfixture — one setup is one schema created and one dbt project written and parsed. 230 is the floor: one per class.--dist load(the pre-#831 default)--dist loadgroup+ marks on the 7 candidate classes--dist worksteal--dist loadscopeEvery mode reported the same results — 356 passed, 48 skipped, 2 xfailed — so this is a scheduling change only.
loadscopeis the only mode that reaches the floor, and it is also the fastest. The tradeoff the issue anticipated does not appear: 230 work units across 12 workers leaves plenty of slack to balance with, and the redundant setups cost more than the coarser scheduling saves. It also needs no list of classes, so it cannot go stale.Two things the issue got wrong, both worth recording
loadgroupis worse than doing nothing for anything unmarked. The issue says it "schedules unmarked tests exactly asloaddoes". It does not.LoadGroupSchedulingisLoadScopeSchedulingwith an unmarked test's scope set to its own nodeid, so every unmarked test becomes its own work unit, handed out one at a time from a queue. Plainloadsends batches of consecutive tests, deliberately — xdist's own comment there says it is preserving the collection order that minimizes "the number of necessary fixture setup/teardown". Marking seven classes saved 7 duplicate setups and gave up about 40 elsewhere. That is a cost #831 introduced suite-wide to fix one class, and this PR removes it.Hand-marking would not have converged. The classes that actually duplicate are not the ones the issue's AST scan predicted. Worst offenders under
load:test_query_options.py::TestQueryOptionsCoretest_table_refresh_method.py::TestDmlRefreshtest_query_options.py::TestQueryOptionsIncrementaltest_index_config.py::TestSQLServerIndexAdvancedtest_full_refresh_build.py::TestFullRefreshBuildtest_openquery.py::TestOpenqueryNeither of the top two has a class-scoped fixture calling
run_dbt, so the scan cannot see them — they are just classes with many tests, and every copy still pays for a schema and a dbt parse. Three of the seven listed classes did not duplicate at all in round 1: which classes split moves run to run with worker timing.What changed
pytest.ini:--dist loadgroup→--dist loadscope, with the table above recorded in the comment so the next person does not have to re-run it.test_openquery.py: drops the now-inertxdist_groupmark test(openquery): bootstrap the loopback linked server, and stop rebuilding it per worker #831 added. Underloadscopethe class is a work unit either way; leaving the mark would leave its docstring crediting a mechanism that is no longer doing the work. Behaviour is unchanged — that is the point.test_catalog.py: a correctness fix the audit turned up, below.secondary_db: the hazard no--distmode fixesTestCatalogAcrossDatabases(tests/functional/adapter/dbt/test_catalog.py) andTestCrossDB(tests/functional/adapter/mssql/test_cross_db.py) each created a database namedsecondary_db, and each dropped it when done — withSET SINGLE_USER WITH ROLLBACK IMMEDIATE, which boots whatever sessions are still on it. A database is instance-wide, so this is #833's linked-server hazard in another shape: two holders of one name, and whoever finishes first pulls it out from under the other.They live in different modules, so no
--distmode separates them.loadscopegroups within a class;loadgroupcould put both classes in one group, but that scheduler is what this PR is removing. Giving the catalog test its own name fixes it without depending on scheduling at all. Not observed failing — each class holds a single test, so the overlap window is one test long — which is why it is worth closing before it is.test_db_non_standard.pyalso creates an instance-wide database, but under a name nothing else uses and it never drops it. Nothing to do there.The rest of #834's inventory
The issue asked about two further groups;
loadscopecovers both without any per-class work, which is most of the argument for it.TestQueryOptionsCoreandTestDmlRefresh, i.e. the actual worst offenders.Verification
workstealrounds, all356 passed, 48 skipped, 2 xfailed.--distoverride, i.e. what CI will do: 230 setups, no class set up twice, 324s, same results.test_catalog.py,test_cross_db.py,test_db_non_standard.py,test_openquery.pytogether at-n 4: 16 passed.load, 3.7s underloadscope.pre-commit runclean on all three changed files.🤖 Generated with Claude Code