diff --git a/src/lightning/fabric/utilities/cloud_io.py b/src/lightning/fabric/utilities/cloud_io.py index fd3196202f8e2..dee220efb3f85 100644 --- a/src/lightning/fabric/utilities/cloud_io.py +++ b/src/lightning/fabric/utilities/cloud_io.py @@ -120,6 +120,7 @@ def _atomic_save(checkpoint: dict[str, Any], filepath: _PATH) -> None: raise RuntimeError( 'Upgrade fsspec to enable cross-device local checkpoints: pip install "fsspec[http]>=2025.5.0"', ) from e + raise def _is_object_storage(fs: AbstractFileSystem) -> bool: diff --git a/tests/tests_fabric/utilities/test_cloud_io.py b/tests/tests_fabric/utilities/test_cloud_io.py index 7e934e5dc86b4..d128240846d87 100644 --- a/tests/tests_fabric/utilities/test_cloud_io.py +++ b/tests/tests_fabric/utilities/test_cloud_io.py @@ -11,10 +11,12 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import errno import os from unittest import mock import fsspec +import pytest import torch from fsspec.implementations.local import LocalFileSystem from fsspec.spec import AbstractFileSystem @@ -150,3 +152,36 @@ def test_atomic_save_uses_write_for_local(tmp_path): assert filepath.exists() loaded = torch.load(filepath, weights_only=True) torch.testing.assert_close(loaded["key"], checkpoint["key"]) + + +def test_atomic_save_permission_error_propagation(): + """Test that _atomic_save propagates PermissionError if it is not an EXDEV error.""" + checkpoint = {"key": torch.tensor([1, 2, 3])} + filepath = "memory://checkpoint.ckpt" + + mock_fs = mock.MagicMock() + mock_fs.open.side_effect = PermissionError("Permission denied") + mock_fs.pipe.side_effect = PermissionError("Permission denied") + + with mock.patch("fsspec.core.url_to_fs", return_value=(mock_fs, "checkpoint.ckpt")): + with pytest.raises(PermissionError, match="Permission denied"): + _atomic_save(checkpoint, filepath) + + +def test_atomic_save_exdev_to_runtime_error(): + """Test that _atomic_save maps PermissionError with EXDEV context to RuntimeError.""" + checkpoint = {"key": torch.tensor([1, 2, 3])} + filepath = "memory://checkpoint.ckpt" + + mock_fs = mock.MagicMock() + + os_error = OSError(errno.EXDEV, "Cross-device link") + permission_error = PermissionError("Permission denied") + permission_error.__context__ = os_error + + mock_fs.open.side_effect = permission_error + mock_fs.pipe.side_effect = permission_error + + with mock.patch("fsspec.core.url_to_fs", return_value=(mock_fs, "checkpoint.ckpt")): + with pytest.raises(RuntimeError, match="Upgrade fsspec to enable cross-device local checkpoints"): + _atomic_save(checkpoint, filepath) diff --git a/tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py b/tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py index 1450a92ed2744..59c76602d90bb 100644 --- a/tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py +++ b/tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py @@ -118,19 +118,23 @@ def test_local_cross_device_checkpoint(tmpdir): ) trainer.fit(model) # Simulate the behavior of fsspec when writing to a local file system but other device. - with ( - mock.patch("os.rename", side_effect=OSError(errno.EXDEV, "Invalid cross-device link")), - mock.patch("os.chmod", side_effect=PermissionError("Operation not permitted")), - ): - if compare_version("fsspec", operator.lt, "2025.5.0"): - with pytest.raises( + if compare_version("fsspec", operator.lt, "2025.5.0"): + with ( + mock.patch("os.rename", side_effect=OSError(errno.EXDEV, "Invalid cross-device link")), + mock.patch("os.chmod", side_effect=PermissionError("Operation not permitted")), + pytest.raises( RuntimeError, match=re.escape( 'Upgrade fsspec to enable cross-device local checkpoints: pip install "fsspec[http]>=2025.5.0"' ), - ): - trainer.save_checkpoint(tmpdir + "/test_ckpt_for_fsspec/hpc_ckpt.ckpt") - else: + ), + ): + trainer.save_checkpoint(tmpdir + "/test_ckpt_for_fsspec/hpc_ckpt.ckpt") + else: + # New fsspec handles the cross-device fallback internally, so only `os.rename` needs + # mocking. Do not mock `os.chmod` here: fsspec's transaction commit calls it directly + # to fix up permissions after the copy, and a global mock would leak into that call. + with mock.patch("os.rename", side_effect=OSError(errno.EXDEV, "Invalid cross-device link")): trainer.save_checkpoint(tmpdir + "/test_ckpt_for_fsspec/hpc_ckpt.ckpt")