A test file with no tests was switching the units system on at collection (#567) — and CI can now run in parallel

.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:

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
 

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):
     """

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)

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)