diff --git a/.github/workflows/build_uw3_and_test.yaml b/.github/workflows/build_uw3_and_test.yaml index 756002e9c..e7286f99a 100644 --- a/.github/workflows/build_uw3_and_test.yaml +++ b/.github/workflows/build_uw3_and_test.yaml @@ -28,16 +28,19 @@ env: # the UCX transports and retire the whole flake class. UCX_TLS: tcp,sm,self - # WORKERS is deliberately NOT set: the batches run in-process until #567 is - # fixed. Distributing them works and was measured at 4 workers (49m43s -> - # 30m53s) but changes which files share a process, which makes three - # point-locator tests answer in the wrong cell. Setting `WORKERS: 4` here is - # the whole change once #567 lands — and the only change, because - # scripts/test.sh already honours it. + # Distribute the serial batches across worker processes. This was held back + # until #567 was fixed: worker counts change which files share a process, and + # a units test that switched the units system on at import time then reached + # a module-scoped fixture in the point-locator suite before anything reset it. + # That is fixed at source in tests/conftest.py, and the whole suite is green + # at 4, 8 and 16 workers. # - # Note if you do set it: detect nothing. The first attempt derived the count - # inside the job, got 1, and ran xdist with a single worker — a spawn and a - # fresh underworld3 import per batch, parallelising nothing, for 52m37s. + # 4 is the vCPU count of the runner. Do NOT try to detect it: the first + # attempt derived the count inside the job, got 1, and ran xdist with a single + # worker — a spawn and a fresh underworld3 import per batch, parallelising + # nothing, for 52m37s. The MPI batches are unaffected either way; they run + # their own mpirun. + WORKERS: 4 jobs: test: diff --git a/scripts/test.sh b/scripts/test.sh index 9e2521759..c60bd39dd 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -56,25 +56,23 @@ export OMP_NUM_THREADS=1 export OPENBLAS_NUM_THREADS=1 export MKL_NUM_THREADS=1 -# CI runs the batches in-process, deliberately. Distributing them across -# workers WORKS and is measured — 49m43s to 30m53s at 4 workers on the -# runner — but it also changes which files share a process, and that exposes -# a real defect: three point-locator tests then answer in a cell that does -# not contain the query point, for every point (issue #567). We are not -# marking those xfail to buy the speedup. +# WORKERS distributes the serial batches, one file at a time per worker. It was +# held back while #567 was open: the worker count decides which files share a +# process, and a units test that switched the units system on at IMPORT time +# then reached a module-scoped fixture in the point-locator suite before +# anything reset it. Fixed at source in tests/conftest.py, so this is now just +# a speed knob. Measured here, ./scripts/test.sh --p 2 on 16 cores: # -# So CI stays serial until #567 is fixed. The developer loop does use workers -# (scripts/test_levels.sh, `./uw test`: 9:45 to 1:22), because its grouping -# does not hit the defect and the fast feedback is what stops people skipping -# tests. Turning CI on afterwards is this block plus WORKERS in the workflow. +# serial batches 25.1 min -> 9.6 min at 8 workers +# end to end 28:04 -> 12:21 # -# WORKERS is honoured if set, so the parallel run stays one env var away for -# anyone bisecting #567 in CI. +# Unset (or 1) still runs everything in one process, which is what you want +# when a test passes alone and fails in a full run. if [ -n "$WORKERS" ] && [ "$WORKERS" -gt 1 ]; then - echo "Serial batches: $WORKERS worker process(es) (WORKERS set; see #567)" + echo "Serial batches: $WORKERS worker process(es)" PYTEST="pytest --config-file=tests/pytest.ini --dist loadfile -n $WORKERS" else - echo "Serial batches: in-process (workers held back pending #567)" + echo "Serial batches: in-process (set WORKERS=N to distribute)" PYTEST="pytest --config-file=tests/pytest.ini" fi diff --git a/tests/conftest.py b/tests/conftest.py index 2762656c6..1485a36a5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -47,6 +47,38 @@ os.environ.setdefault("UW_MESH_CACHE_DIR", f".meshes/{_xdist_worker}") +@pytest.fixture(scope="module", autouse=True) +def isolate_module_state(): + """Reset the global model BEFORE a module's own fixtures are built. + + ``isolate_test_state`` below runs per test, and a per-test fixture cannot + protect a module-scoped one: pytest builds higher-scoped fixtures first, so + a ``scope="module"`` fixture that creates a mesh is set up BEFORE the first + test's function-scoped reset ever runs. Anything the process did earlier — + a previous module's last test, or module-level code executed during + COLLECTION — is therefore still in force while that mesh is built. + + That is #567. ``test_0741_expression_arithmetic_units.py`` used to call + ``set_reference_quantities`` at import time, which switches the units + system on globally. ``test_0761_point_locator.py`` builds its mesh and P1 + variable in a module-scoped fixture, and with units active ``var.coords`` + returns DIMENSIONAL coordinates (0..2.9e6 m rather than the mesh's 0..1), + so the fixture wrote nodal values sampled at the wrong points and every + later evaluation disagreed with the closed form by O(1). It only bit when + that file happened to be the FIRST in its process — which under + ``--dist loadfile`` is decided by the worker count, and never happens in a + serial run because an earlier file's per-test reset has already cleaned up. + + Resetting at module scope closes that window for every module-scoped + fixture in the suite, not just the one that exposed it. + """ + import underworld3 as uw + + uw.reset_default_model() + yield + uw.reset_default_model() + + @pytest.fixture(scope="function", autouse=True) def isolate_test_state(request): """ diff --git a/tests/test_0741_expression_arithmetic_units.py b/tests/test_0741_expression_arithmetic_units.py index 25fceec97..7f0a4e12c 100644 --- a/tests/test_0741_expression_arithmetic_units.py +++ b/tests/test_0741_expression_arithmetic_units.py @@ -1,89 +1,69 @@ -import pytest +"""Units propagate through products of quantities and expressions. -# Units system tests - intermediate complexity -pytestmark = pytest.mark.level_2 -#!/usr/bin/env python3 -""" -Test units propagation in UWexpression arithmetic. +``cm/year * Myr`` is a length however the two factors are spelled — as +:func:`uw.quantity` (a value with units) or as :func:`uw.expression` (a named +symbol carrying units) — and in either order. Four spellings of the same +product, all of which must come out in centimetres. + +The reference quantities live in a FIXTURE, not at module level. This file used +to run its setup at import time, which meant ``set_reference_quantities`` ran +during pytest COLLECTION and switched the units system on globally before a +single test executed. Any module whose own fixture is module-scoped is then +built with units active — and ``var.coords`` returns dimensional coordinates +when they are — which is how a units file broke the point-locator suite +(#567). Global state belongs inside a fixture that takes it down again. """ +import pytest import underworld3 as uw -print("=" * 70) -print("Testing Units Propagation in UWexpression Arithmetic") -print("=" * 70) - -# Setup -model = uw.Model() -model.set_reference_quantities( - length=uw.quantity(2900, "km"), - time=uw.quantity(1, "Myr"), -) - -# Test 1: Quantity * Quantity -print("\n1. UWQuantity * UWQuantity:") -v1 = uw.quantity(5, "cm/year") -t1 = uw.quantity(1, "Myr") - -result1 = v1 * t1 -print(f" {v1.units} * {t1.units}") -print(f" Result: {uw.get_units(result1)}") -print(f" Expected: centimeter") -print(f" ✓ Correct!" if "centimeter" in str(uw.get_units(result1)) else " ✗ WRONG!") - -# Test 2: Quantity * Expression -print("\n2. UWQuantity * UWexpression:") -v2 = uw.quantity(5, "cm/year") -t2 = uw.expression(r"t_\textrm{now}", 1, "Current time", units="Myr") - -print(f" velocity: {v2} (type: {type(v2).__name__})") -print(f" time: {t2} (type: {type(t2).__name__})") -print(f" velocity units: {uw.get_units(v2)}") -print(f" time units: {uw.get_units(t2)}") - -result2 = v2 * t2 -print(f" Result: {result2}") -print(f" Result type: {type(result2).__name__}") -print(f" Result units: {uw.get_units(result2)}") -print(f" Expected: centimeter (cm/year * Myr = cm)") -print(f" ✓ Correct!" if "centimeter" in str(uw.get_units(result2)) else " ✗ WRONG!") - -# Test 3: Expression * Quantity (reverse) -print("\n3. UWexpression * UWQuantity (reverse order):") -result3 = t2 * v2 -print(f" Result: {result3}") -print(f" Result type: {type(result3).__name__}") -print(f" Result units: {uw.get_units(result3)}") -print(f" Expected: centimeter") -print(f" ✓ Correct!" if "centimeter" in str(uw.get_units(result3)) else " ✗ WRONG!") - -# Test 4: Expression * Expression -print("\n4. UWexpression * UWexpression:") -v3 = uw.expression("v", 5, "velocity", units="cm/year") -t3 = uw.expression("t", 1, "time", units="Myr") - -result4 = v3 * t3 -print(f" Result: {result4}") -print(f" Result type: {type(result4).__name__}") -print(f" Result units: {uw.get_units(result4)}") -print(f" Expected: centimeter") -print(f" ✓ Correct!" if "centimeter" in str(uw.get_units(result4)) else " ✗ WRONG!") - -# Test 5: Check if it's a SymPy vs Pint issue -print("\n5. Internal state inspection:") -v_qty = uw.quantity(5, "cm/year") -t_expr = uw.expression("t", 1, "time", units="Myr") -product = v_qty * t_expr - -print(f" Product type: {type(product)}") -print(f" Has _pint_qty? {hasattr(product, '_pint_qty')}") -if hasattr(product, '_pint_qty'): - print(f" _pint_qty: {product._pint_qty}") - print(f" _pint_qty.units: {product._pint_qty.units}") -print(f" Has .sym? {hasattr(product, 'sym')}") -if hasattr(product, 'sym'): - print(f" .sym: {product.sym}") - print(f" get_units(product.sym): {uw.get_units(product.sym) if hasattr(product, 'sym') else 'N/A'}") - -print("\n" + "=" * 70) -print("Summary") -print("=" * 70) +pytestmark = [pytest.mark.level_2, pytest.mark.tier_a] + + +@pytest.fixture +def mantle_scaling(): + """A model with a length and a time scale, torn down after the test.""" + orchestration_model = uw.Model() + orchestration_model.set_reference_quantities( + length=uw.quantity(2900, "km"), + time=uw.quantity(1, "Myr"), + ) + yield orchestration_model + uw.reset_default_model() + + +def _is_length_in_cm(product): + return "centimeter" in str(uw.get_units(product)) + + +def test_quantity_times_quantity_is_a_length(mantle_scaling): + velocity = uw.quantity(5, "cm/year") + interval = uw.quantity(1, "Myr") + assert _is_length_in_cm(velocity * interval) + + +def test_quantity_times_expression_is_a_length(mantle_scaling): + velocity = uw.quantity(5, "cm/year") + interval = uw.expression(r"t_\textrm{now}", 1, "Current time", units="Myr") + assert _is_length_in_cm(velocity * interval) + + +def test_expression_times_quantity_is_a_length(mantle_scaling): + """The reverse order goes through ``__rmul__`` and used to lose the units.""" + velocity = uw.quantity(5, "cm/year") + interval = uw.expression(r"t_\textrm{now}", 1, "Current time", units="Myr") + assert _is_length_in_cm(interval * velocity) + + +def test_expression_times_expression_is_a_length(mantle_scaling): + velocity = uw.expression("v", 5, "velocity", units="cm/year") + interval = uw.expression("t", 1, "time", units="Myr") + assert _is_length_in_cm(velocity * interval) + + +def test_the_product_carries_its_units_onto_its_symbol(mantle_scaling): + """The units must survive the trip to ``.sym``, which is what the solvers + and the JIT compiler actually read.""" + product = uw.quantity(5, "cm/year") * uw.expression( + "t", 1, "time", units="Myr") + assert hasattr(product, "sym") + assert _is_length_in_cm(product.sym) diff --git a/tests/test_0742_module_fixture_units_isolation.py b/tests/test_0742_module_fixture_units_isolation.py new file mode 100644 index 000000000..9652ebf65 --- /dev/null +++ b/tests/test_0742_module_fixture_units_isolation.py @@ -0,0 +1,76 @@ +"""A module-scoped fixture must be built with the global model reset (#567). + +``conftest.isolate_test_state`` resets the default model before every test, and +that reads like enough isolation. It is not. pytest builds higher-scoped +fixtures first, so a ``scope="module"`` fixture — the natural place to put a +mesh several tests share — is set up BEFORE the first test's function-scoped +reset. Whatever the process did earlier is still in force while that mesh is +built, including module-level code that ran during COLLECTION. + +That is how a units test broke the point locator. With the units system active, +``var.coords`` returns DIMENSIONAL coordinates: for a 2900 km length scale, the +unit box reads 0..2.9e6 instead of 0..1. A fixture that fills nodal values from +``var.coords`` then samples its field at points 2.9 million times too far +apart, and every later evaluation disagrees with the closed form by O(1) — a +failure that looks like a broken locator and is nothing of the sort. + +It only bit when the affected file happened to run FIRST in its process, which +under ``pytest --dist loadfile`` is decided by the worker count. Hence a suite +that was green serially and at 4 and 8 workers, and red at 16. + +The check runs a real pytest in a subprocess against a copy of the live +``conftest.py``, because the thing under test IS that conftest: delete the +module-scoped reset from it and this test fails. +""" +from pathlib import Path + +import pytest + +pytestmark = [pytest.mark.level_1, pytest.mark.tier_a] + +pytest_plugins = ["pytester"] + +_LIVE_CONFTEST = (Path(__file__).parent / "conftest.py").read_text() + +# The polluter and the victim, in one file. Module-level code runs at +# collection, exactly as an import-time `set_reference_quantities` does; the +# module-scoped fixture is the victim. Deliberately NOT run in this process — +# it would be the very leak we are pinning against. +_LEAKING_MODULE = ''' +import numpy as np +import pytest +import underworld3 as uw + +# Import-time global state, as test_0741 used to have. +_orchestration_model = uw.Model() +_orchestration_model.set_reference_quantities( + length=uw.quantity(2900, "km"), + time=uw.quantity(1, "Myr"), +) + + +@pytest.fixture(scope="module") +def coordinate_extent(): + """A module-scoped mesh, and how far its variable coordinates reach.""" + mesh = uw.meshing.UnstructuredSimplexBox( + minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), + cellSize=0.5, regular=False, qdegree=2) + field = uw.discretisation.MeshVariable("u_iso", mesh, 1, degree=1) + return float(np.asarray(field.coords).max()) + + +def test_the_fixture_saw_the_undimensionalised_unit_box(coordinate_extent): + assert coordinate_extent == pytest.approx(1.0), ( + f"the module-scoped fixture read coordinates reaching " + f"{coordinate_extent:.6g} on a unit box — it was built while the " + f"units system was still switched on by import-time code") +''' + + +def test_a_module_scoped_fixture_is_not_built_under_leaked_units(pytester): + pytester.makeconftest(_LIVE_CONFTEST) + pytester.makepyfile(test_leaking_module=_LEAKING_MODULE) + + result = pytester.runpytest_subprocess("-q", "-p", "no:cacheprovider") + + result.assert_outcomes(passed=1)