-
Notifications
You must be signed in to change notification settings - Fork 71
perf(mode): pin sliced mode-solver plane to single-device sharding #401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| from fdtdx.core.axis import get_transverse_axes | ||
| from fdtdx.core.grid import calculate_time_offset_yee | ||
| from fdtdx.core.jax.pytrees import autoinit, frozen_field, private_field | ||
| from fdtdx.core.jax.sharding import pin_to_single_device | ||
| from fdtdx.core.linalg import get_wave_vector_raw | ||
| from fdtdx.core.physics.metrics import compute_energy | ||
| from fdtdx.core.physics.modes import compute_mode | ||
|
|
@@ -112,10 +113,11 @@ def apply( | |
| raise NotImplementedError() | ||
|
|
||
| # inv_permittivities shape: (3, Nx, Ny, Nz) - slice with component dimension | ||
| inv_permittivity_slice = inv_permittivities[:, *self.grid_slice] | ||
| # Pin the sliced plane to a single device (see pin_to_single_device docstring for why). | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. references to other comments are generally not good style |
||
| inv_permittivity_slice = pin_to_single_device(inv_permittivities[:, *self.grid_slice]) | ||
| if isinstance(inv_permeabilities, jax.Array) and inv_permeabilities.ndim > 0: | ||
| # inv_permeabilities shape: (3, Nx, Ny, Nz) - slice with component dimension | ||
| inv_permeability_slice = inv_permeabilities[:, *self.grid_slice] | ||
| inv_permeability_slice = pin_to_single_device(inv_permeabilities[:, *self.grid_slice]) | ||
| else: | ||
| inv_permeability_slice = inv_permeabilities | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| """Unit tests for fdtdx.core.jax.sharding module.""" | ||
|
|
||
| import os | ||
| import subprocess | ||
| import sys | ||
| import textwrap | ||
| from unittest import mock | ||
| from unittest.mock import MagicMock | ||
|
|
||
|
|
@@ -10,9 +14,11 @@ | |
| import fdtdx.core.jax.sharding as sharding_module | ||
| from fdtdx.constants import SHARD_STR | ||
| from fdtdx.core.jax.sharding import ( | ||
| _replicated_mesh_sharding, | ||
| create_named_sharded_matrix, | ||
| get_dtype_bytes, | ||
| get_named_sharding_from_shape, | ||
| pin_to_single_device, | ||
| pretty_print_sharding, | ||
| ) | ||
|
|
||
|
|
@@ -198,3 +204,184 @@ def test_sharding_axis_fallback_when_dim_is_one(self): | |
| def test_has_named_sharding(self): | ||
| result = create_named_sharded_matrix(shape=(4, 6), value=1.0, sharding_axis=0, dtype=jnp.float32, backend="cpu") | ||
| assert isinstance(result.sharding, jax.sharding.NamedSharding) | ||
|
|
||
|
|
||
| # ---- _replicated_mesh_sharding ---- | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove excessive comments and unnecessary spacing |
||
|
|
||
|
|
||
| class TestReplicatedMeshSharding: | ||
| """Tests for the _replicated_mesh_sharding helper, split out of pin_to_single_device so this | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is also convoluted |
||
| (pure, cheap) mesh/sharding construction can be covered in-process -- unlike pin_to_single_device's | ||
| own data placement, it does not require genuinely distinct physical devices, so a MOCKED duplicated | ||
| device list is a faithful, subprocess-free test (the corresponding data placement is exercised | ||
| separately by TestPinToSingleDevice's subprocess-based multi-device tests below).""" | ||
|
|
||
| def test_single_device_replicated_spec(self): | ||
| cpu = CPU_DEVICES[0] | ||
| result = _replicated_mesh_sharding([cpu]) | ||
| assert isinstance(result, jax.sharding.NamedSharding) | ||
| assert tuple(result.spec) == () | ||
| assert result.mesh.devices.shape == (1,) | ||
|
|
||
| def test_duplicated_device_list_replicated_spec(self): | ||
| # A duplicated physical device (simulating >1 "device" without needing real distinct hardware) | ||
| # is accepted by mesh/sharding CONSTRUCTION -- only the actual data placement in | ||
| # pin_to_single_device requires genuine distinctness (see module docstring above). | ||
| cpu = CPU_DEVICES[0] | ||
| result = _replicated_mesh_sharding([cpu, cpu]) | ||
| assert isinstance(result, jax.sharding.NamedSharding) | ||
| assert tuple(result.spec) == () | ||
| assert result.mesh.devices.shape == (2,) | ||
| assert SHARD_STR in result.mesh.axis_names | ||
|
|
||
| def test_result_usable_as_pin_to_single_device_input(self): | ||
| """pin_to_single_device calls this helper directly with jax.devices() -- confirm the two agree | ||
| on shape/axis-name conventions for a real (single, non-mocked) device list.""" | ||
| expected = _replicated_mesh_sharding(jax.devices()) | ||
| assert isinstance(expected, jax.sharding.NamedSharding) | ||
| assert expected.mesh.devices.shape == (len(jax.devices()),) | ||
|
|
||
|
|
||
| # ---- pin_to_single_device ---- | ||
|
|
||
|
|
||
| def _run_in_subprocess_with_devices(num_devices: int, body: str) -> None: | ||
| """Run ``body`` in a fresh interpreter with ``num_devices`` virtual CPU devices. | ||
|
|
||
| ``jax.devices()`` reflects the platform's device count only at JAX's first backend | ||
| initialisation, which has already happened (with a single forced CPU device, see | ||
| ``tests/conftest.py``) by the time this test module runs. A real multi-device mesh can | ||
| therefore only be exercised in a separate process started with | ||
| ``XLA_FLAGS=--xla_force_host_platform_device_count=N`` set beforehand. Mocking | ||
| ``jax.devices()`` to return the same physical device twice is not a substitute here: | ||
| unlike the mesh-shape/spec checks elsewhere in this file, ``pin_to_single_device`` places | ||
| data onto a real device mesh eagerly, which requires genuinely distinct physical devices | ||
| (a mesh built from a duplicated device raises at construction/placement time). | ||
| """ | ||
| env = { | ||
| **os.environ, | ||
| "JAX_PLATFORMS": "cpu", | ||
| "XLA_FLAGS": f"--xla_force_host_platform_device_count={num_devices}", | ||
| } | ||
| result = subprocess.run( | ||
| [sys.executable, "-c", textwrap.dedent(body)], | ||
| capture_output=True, | ||
| text=True, | ||
| env=env, | ||
| timeout=60, | ||
| ) | ||
| assert result.returncode == 0, f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" | ||
|
|
||
|
|
||
| class TestPinToSingleDevice: | ||
| """Tests for the pin_to_single_device helper.""" | ||
|
|
||
| def test_single_device_is_noop_identity(self): | ||
| # The default test session forces a single CPU device (see tests/conftest.py). | ||
| assert len(jax.devices()) == 1 | ||
| arr = jnp.arange(12.0).reshape(3, 4) | ||
| result = pin_to_single_device(arr) | ||
| assert result is arr | ||
|
|
||
| def test_multi_device_preserves_shape_dtype_and_values(self): | ||
| _run_in_subprocess_with_devices( | ||
| 2, | ||
| """ | ||
| import jax, jax.numpy as jnp | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any way to run these unit tests without having python code as strings? This kind of code is hard to maintain, maybe a better variant would be defining a helper function directly as python code and executing that |
||
| from fdtdx.core.jax.sharding import pin_to_single_device | ||
|
|
||
| assert len(jax.devices()) == 2 | ||
| arr = jnp.arange(24.0, dtype=jnp.float32).reshape(4, 6) | ||
| out = pin_to_single_device(arr) | ||
| assert out.shape == arr.shape | ||
| assert out.dtype == arr.dtype | ||
| assert bool(jnp.array_equal(out, arr)) | ||
| """, | ||
| ) | ||
|
|
||
| def test_multi_device_result_is_replicated_named_sharding(self): | ||
| _run_in_subprocess_with_devices( | ||
| 2, | ||
| """ | ||
| import jax, jax.numpy as jnp | ||
| from fdtdx.core.jax.sharding import pin_to_single_device | ||
|
|
||
| out = pin_to_single_device(jnp.zeros((4, 6), dtype=jnp.float32)) | ||
| assert isinstance(out.sharding, jax.sharding.NamedSharding) | ||
| # Fully replicated: an empty PartitionSpec, not sharded along any axis. | ||
| assert tuple(out.sharding.spec) == () | ||
| assert "shard" in out.sharding.mesh.axis_names | ||
| """, | ||
| ) | ||
|
|
||
| def test_multi_device_compatible_with_sliced_x_sharded_parent(self): | ||
| """Regression test for the exact usage in ModePlaneSource / ModeOverlapDetector: | ||
| pinning a plane sliced out of an x-sharded array inside a jit must not raise JAX's | ||
| "Received incompatible devices for jitted computation" error, which is what happens | ||
| if the sliced plane were instead placed on a plain SingleDeviceSharding (device 0) | ||
| while the surrounding computation is compiled over the full device mesh. | ||
| """ | ||
| _run_in_subprocess_with_devices( | ||
| 2, | ||
| """ | ||
| import jax, jax.numpy as jnp | ||
| from fdtdx.core.jax.sharding import create_named_sharded_matrix, pin_to_single_device | ||
|
|
||
| # (component, Nx, Ny, Nz) permittivity-like array, x-sharded on axis=1. | ||
| arr = create_named_sharded_matrix( | ||
| (3, 8, 6, 6), value=2.0, dtype=jnp.float32, sharding_axis=1, backend="cpu" | ||
| ) | ||
|
|
||
| @jax.jit | ||
| def slice_and_pin(a): | ||
| plane = a[:, 2:3, :, :] | ||
| return pin_to_single_device(plane) | ||
|
|
||
| out = slice_and_pin(arr) | ||
| assert out.shape == (3, 1, 6, 6) | ||
| assert bool(jnp.allclose(out, 2.0)) | ||
| """, | ||
| ) | ||
|
|
||
| def test_multi_device_compatible_with_pure_callback_output(self): | ||
| """Regression test for compute_mode's usage: pinning the RAW OUTPUT of a | ||
| jax.pure_callback (as returned by the Tidy3D mode solver) must not raise "Cannot | ||
| convert GSPMDSharding {maximal device=0} into SdyArray". A pure_callback's result | ||
| carries a maximal/single-device GSPMDSharding -- a fundamentally different | ||
| representation than the ordinary NamedSharding case covered by the sliced-input | ||
| test above, which XLA's Shardy partitioner cannot convert via | ||
| jax.lax.with_sharding_constraint (this failed before this fix; device_put succeeds | ||
| because it performs the placement directly instead of a Shardy-mediated re-shard). | ||
| """ | ||
| _run_in_subprocess_with_devices( | ||
| 2, | ||
| """ | ||
| import numpy as np | ||
| import jax, jax.numpy as jnp | ||
| from fdtdx.core.jax.sharding import create_named_sharded_matrix, pin_to_single_device | ||
|
|
||
| arr = create_named_sharded_matrix( | ||
| (3, 8, 6, 6), value=2.0, dtype=jnp.float32, sharding_axis=1, backend="cpu" | ||
| ) | ||
|
|
||
| def host_fn(x): | ||
| # Stand-in for the Tidy3D mode-solver call in compute_mode: an arbitrary | ||
| # host-side computation returning a NEW array (not just a passthrough). | ||
| return np.asarray(x) * 3.0 | ||
|
|
||
| @jax.jit | ||
| def slice_callback_and_pin(a): | ||
| plane = a[:, 2:3, :, :] | ||
| raw = jax.pure_callback( | ||
| host_fn, jax.ShapeDtypeStruct(plane.shape, plane.dtype), plane | ||
| ) | ||
| pinned = pin_to_single_device(raw) | ||
| # Mirrors compute_mode's jnp.expand_dims(mode_E_raw, ...) right after pinning -- | ||
| # the exact next op that raised before this fix. | ||
| return jnp.expand_dims(pinned, axis=0) | ||
|
|
||
| out = slice_callback_and_pin(arr) | ||
| assert out.shape == (1, 3, 1, 6, 6) | ||
| assert bool(jnp.allclose(out, 6.0)) | ||
| """, | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all of the AI chain-of-thoughts comments. We want to have actual docstrings, not a development story