From 90bee093d5f1e61a9607f445b17f28aab4047380 Mon Sep 17 00:00:00 2001 From: Shriprasad R Patil Date: Sat, 12 Sep 2026 19:21:26 +0000 Subject: [PATCH 1/3] Fix S3 client to use system temp directory instead of CWD The S3 client was using the current working directory (CWD) as the default location for temporary files. This caused issues when the CWD was read-only or not writable. Changed TEMPDIR default from '.' to tempfile.gettempdir() which uses the system's temporary directory (typically /tmp on Unix systems). Added test to verify S3 client works correctly in read-only directories. Fixes #854 --- metaflow/metaflow_config.py | 3 +- test/unit/test_s3_readonly_cwd.py | 48 +++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 test/unit/test_s3_readonly_cwd.py diff --git a/metaflow/metaflow_config.py b/metaflow/metaflow_config.py index 97be8a38056..5b3ab40f0c3 100644 --- a/metaflow/metaflow_config.py +++ b/metaflow/metaflow_config.py @@ -1,5 +1,6 @@ import os import sys +import tempfile import types import uuid import datetime @@ -176,7 +177,7 @@ ), ) -TEMPDIR = from_conf("TEMPDIR", ".") +TEMPDIR = from_conf("TEMPDIR", tempfile.gettempdir()) DATATOOLS_CLIENT_PARAMS = from_conf("DATATOOLS_CLIENT_PARAMS", {}) if S3_ENDPOINT_URL: diff --git a/test/unit/test_s3_readonly_cwd.py b/test/unit/test_s3_readonly_cwd.py new file mode 100644 index 00000000000..47f5e821c96 --- /dev/null +++ b/test/unit/test_s3_readonly_cwd.py @@ -0,0 +1,48 @@ +"""Test that S3 client works when current directory is not writeable.""" + +import os +import tempfile +import pytest + + +def test_s3_client_readonly_cwd(mocker): + """Test that S3 client doesn't require writable CWD (issue #854).""" + # Mock boto3 and dependencies before importing S3 + mock_boto3 = mocker.MagicMock() + mock_transfer_config = mocker.MagicMock() + mocker.patch.dict("sys.modules", {"boto3": mock_boto3}) + mocker.patch.dict( + "sys.modules", + {"boto3.s3.transfer": mocker.MagicMock(TransferConfig=mock_transfer_config)}, + ) + + from metaflow.plugins.datatools.s3 import S3 + + # Create a temporary read-only directory + with tempfile.TemporaryDirectory() as tmpdir: + readonly_dir = os.path.join(tmpdir, "readonly") + os.makedirs(readonly_dir, mode=0o555) + + # Change to read-only directory + original_cwd = os.getcwd() + try: + os.chdir(readonly_dir) + + # This should not raise an exception about permissions + # The S3 client should use system temp dir, not CWD + s3_client = S3() + + # Verify that tmpdir was created in system temp, not CWD + assert s3_client._tmpdir is not None + # The temp dir should NOT be in the current (read-only) directory + assert not s3_client._tmpdir.startswith(readonly_dir) + # The temp dir should be in the system temp directory + system_temp = tempfile.gettempdir() + assert s3_client._tmpdir.startswith(system_temp) + + # Clean up + s3_client.close() + finally: + os.chdir(original_cwd) + # Make directory writable again so it can be deleted + os.chmod(readonly_dir, 0o755) From e5fd50e36f33b161900f0a560f9ef2cac94d006b Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 14 Sep 2026 08:39:56 +0530 Subject: [PATCH 2/3] Document and test Metaflow tempdir defaults --- metaflow/plugins/aws/batch/batch_decorator.py | 5 +- metaflow/plugins/datatools/s3/s3.py | 7 +- .../kubernetes/kubernetes_decorator.py | 5 +- test/unit/test_s3_readonly_cwd.py | 107 +++++++++++++----- 4 files changed, 88 insertions(+), 36 deletions(-) diff --git a/metaflow/plugins/aws/batch/batch_decorator.py b/metaflow/plugins/aws/batch/batch_decorator.py index 17057478823..d42d9b021ee 100644 --- a/metaflow/plugins/aws/batch/batch_decorator.py +++ b/metaflow/plugins/aws/batch/batch_decorator.py @@ -299,8 +299,9 @@ def task_pre_step( self.metadata = metadata self.task_datastore = task_datastore - # current.tempdir reflects the value of METAFLOW_TEMPDIR (the current working - # directory by default), or the value of tmpfs_path if tmpfs_tempdir=False. + # current.tempdir reflects the value of METAFLOW_TEMPDIR (the system + # temporary directory from tempfile.gettempdir() by default), or the + # value of tmpfs_path if tmpfs_tempdir=False. if not self.attributes["tmpfs_tempdir"]: current._update_env({"tempdir": self.attributes["tmpfs_path"]}) diff --git a/metaflow/plugins/datatools/s3/s3.py b/metaflow/plugins/datatools/s3/s3.py index a31cb6eafa6..f84d8e31ab4 100644 --- a/metaflow/plugins/datatools/s3/s3.py +++ b/metaflow/plugins/datatools/s3/s3.py @@ -479,8 +479,9 @@ class S3(object): data = [obj.blob for obj in s3.get_many(urls)] s3.close() ``` - You can customize the location of the temporary directory with `tmproot`. It - defaults to the current working directory. + You can customize the location of the temporary directory with `tmproot`, or + with the `METAFLOW_TEMPDIR` configuration variable. If neither is specified, + the system temporary directory returned by `tempfile.gettempdir()` is used. To make it easier to deal with object locations, the client can be initialized with an S3 path prefix. There are three ways to handle locations: @@ -499,7 +500,7 @@ class S3(object): Parameters ---------- - tmproot : str, default '.' + tmproot : str, default METAFLOW_TEMPDIR or tempfile.gettempdir() Where to store the temporary directory. bucket : str, optional, default None Override the bucket from `DATATOOLS_S3ROOT` when `run` is specified. diff --git a/metaflow/plugins/kubernetes/kubernetes_decorator.py b/metaflow/plugins/kubernetes/kubernetes_decorator.py index f0efba46087..965f93428c3 100644 --- a/metaflow/plugins/kubernetes/kubernetes_decorator.py +++ b/metaflow/plugins/kubernetes/kubernetes_decorator.py @@ -529,8 +529,9 @@ def task_pre_step( self.metadata = metadata self.task_datastore = task_datastore - # current.tempdir reflects the value of METAFLOW_TEMPDIR (the current working - # directory by default), or the value of tmpfs_path if tmpfs_tempdir=False. + # current.tempdir reflects the value of METAFLOW_TEMPDIR (the system + # temporary directory from tempfile.gettempdir() by default), or the + # value of tmpfs_path if tmpfs_tempdir=False. if not self.attributes["tmpfs_tempdir"]: current._update_env({"tempdir": self.attributes["tmpfs_path"]}) diff --git a/test/unit/test_s3_readonly_cwd.py b/test/unit/test_s3_readonly_cwd.py index 47f5e821c96..1faa90d099c 100644 --- a/test/unit/test_s3_readonly_cwd.py +++ b/test/unit/test_s3_readonly_cwd.py @@ -1,13 +1,13 @@ """Test that S3 client works when current directory is not writeable.""" +import importlib import os import tempfile + import pytest -def test_s3_client_readonly_cwd(mocker): - """Test that S3 client doesn't require writable CWD (issue #854).""" - # Mock boto3 and dependencies before importing S3 +def _mock_s3_dependencies(mocker): mock_boto3 = mocker.MagicMock() mock_transfer_config = mocker.MagicMock() mocker.patch.dict("sys.modules", {"boto3": mock_boto3}) @@ -16,33 +16,82 @@ def test_s3_client_readonly_cwd(mocker): {"boto3.s3.transfer": mocker.MagicMock(TransferConfig=mock_transfer_config)}, ) - from metaflow.plugins.datatools.s3 import S3 - # Create a temporary read-only directory - with tempfile.TemporaryDirectory() as tmpdir: - readonly_dir = os.path.join(tmpdir, "readonly") - os.makedirs(readonly_dir, mode=0o555) +def _load_s3_class(): + """Reload configuration and S3 so environment changes affect the default.""" + metaflow_config = importlib.import_module("metaflow.metaflow_config") + s3_module = importlib.import_module("metaflow.plugins.datatools.s3.s3") + importlib.reload(metaflow_config) + importlib.reload(s3_module) + # Keep the package-level export in sync for tests that import S3 later. + s3_package = importlib.import_module("metaflow.plugins.datatools.s3") + s3_package.S3 = s3_module.S3 + return s3_module.S3 - # Change to read-only directory - original_cwd = os.getcwd() - try: - os.chdir(readonly_dir) - # This should not raise an exception about permissions - # The S3 client should use system temp dir, not CWD - s3_client = S3() +def test_s3_client_readonly_cwd(mocker, monkeypatch): + """Test that S3 client doesn't require writable CWD (issue #854).""" + _mock_s3_dependencies(mocker) + original_tempdir = os.environ.get("METAFLOW_TEMPDIR") + + try: + with monkeypatch.context() as env: + env.delenv("METAFLOW_TEMPDIR", raising=False) + S3 = _load_s3_class() + + # Create a temporary read-only directory + with tempfile.TemporaryDirectory() as tmpdir: + readonly_dir = os.path.join(tmpdir, "readonly") + os.makedirs(readonly_dir, mode=0o555) + + # Change to read-only directory + original_cwd = os.getcwd() + try: + os.chdir(readonly_dir) - # Verify that tmpdir was created in system temp, not CWD - assert s3_client._tmpdir is not None - # The temp dir should NOT be in the current (read-only) directory - assert not s3_client._tmpdir.startswith(readonly_dir) - # The temp dir should be in the system temp directory - system_temp = tempfile.gettempdir() - assert s3_client._tmpdir.startswith(system_temp) - - # Clean up - s3_client.close() - finally: - os.chdir(original_cwd) - # Make directory writable again so it can be deleted - os.chmod(readonly_dir, 0o755) + # This should not raise an exception about permissions + # The S3 client should use system temp dir, not CWD + s3_client = S3() + + # Verify that tmpdir was created in system temp, not CWD + assert s3_client._tmpdir is not None + # The temp dir should NOT be in the current (read-only) directory + assert not s3_client._tmpdir.startswith(readonly_dir) + # The temp dir should be in the system temp directory + system_temp = tempfile.gettempdir() + assert s3_client._tmpdir.startswith(system_temp) + + # Clean up + s3_client.close() + finally: + os.chdir(original_cwd) + # Make directory writable again so it can be deleted + os.chmod(readonly_dir, 0o755) + assert os.environ.get("METAFLOW_TEMPDIR") == original_tempdir + finally: + # Restore the S3 module's import-time default after the isolated test. + _load_s3_class() + + +def test_s3_client_respects_metaflow_tempdir(mocker, monkeypatch, tmp_path): + """Test that METAFLOW_TEMPDIR overrides the system temporary directory.""" + _mock_s3_dependencies(mocker) + configured_dir = tmp_path / "configured" + configured_dir.mkdir() + original_tempdir = os.environ.get("METAFLOW_TEMPDIR") + + try: + with monkeypatch.context() as env: + env.setenv("METAFLOW_TEMPDIR", str(configured_dir)) + S3 = _load_s3_class() + + s3_client = S3() + try: + assert s3_client._tmproot == str(configured_dir) + assert os.path.dirname(s3_client._tmpdir) == str(configured_dir) + finally: + s3_client.close() + assert os.environ.get("METAFLOW_TEMPDIR") == original_tempdir + finally: + # Restore the S3 module's import-time default after the isolated test. + _load_s3_class() From 47c83ab8a5edb987b66196a79e1ffd2db47a89e2 Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 14 Sep 2026 08:56:28 +0530 Subject: [PATCH 3/3] Isolate S3 tempdir tests in subprocesses --- test/unit/test_s3_readonly_cwd.py | 174 +++++++++++++++++------------- 1 file changed, 97 insertions(+), 77 deletions(-) diff --git a/test/unit/test_s3_readonly_cwd.py b/test/unit/test_s3_readonly_cwd.py index 1faa90d099c..44e6d1e7c55 100644 --- a/test/unit/test_s3_readonly_cwd.py +++ b/test/unit/test_s3_readonly_cwd.py @@ -1,97 +1,117 @@ """Test that S3 client works when current directory is not writeable.""" -import importlib import os -import tempfile +from pathlib import Path +import subprocess +import sys +import textwrap + + +S3_SUBPROCESS_SCRIPT = textwrap.dedent( + """ + import os + import sys + import tempfile + from types import ModuleType + + # Keep this test independent of the optional boto3 dependency. + boto3 = ModuleType("boto3") + boto3_s3 = ModuleType("boto3.s3") + transfer = ModuleType("boto3.s3.transfer") + + class TransferConfig: + multipart_threshold = 8 * 1024 * 1024 + + transfer.TransferConfig = TransferConfig + boto3.s3 = boto3_s3 + boto3_s3.transfer = transfer + sys.modules.update( + { + "boto3": boto3, + "boto3.s3": boto3_s3, + "boto3.s3.transfer": transfer, + } + ) -import pytest + import metaflow + from metaflow.plugins.datatools import S3 as DatatoolsS3 + from metaflow.plugins.datatools.s3 import S3 + from metaflow.plugins.datastores.s3_storage import S3 as StorageS3 + assert metaflow.S3 is DatatoolsS3 is S3 is StorageS3 -def _mock_s3_dependencies(mocker): - mock_boto3 = mocker.MagicMock() - mock_transfer_config = mocker.MagicMock() - mocker.patch.dict("sys.modules", {"boto3": mock_boto3}) - mocker.patch.dict( - "sys.modules", - {"boto3.s3.transfer": mocker.MagicMock(TransferConfig=mock_transfer_config)}, - ) + s3_client = S3() + try: + expected_tempdir = os.environ.get("TEST_EXPECTED_METAFLOW_TEMPDIR") + if expected_tempdir is None: + assert s3_client._tmproot == tempfile.gettempdir() + assert s3_client._tmpdir.startswith(tempfile.gettempdir()) + assert not s3_client._tmpdir.startswith(os.getcwd()) + else: + assert s3_client._tmproot == expected_tempdir + assert os.path.dirname(s3_client._tmpdir) == expected_tempdir + finally: + s3_client.close() + """ +) -def _load_s3_class(): - """Reload configuration and S3 so environment changes affect the default.""" - metaflow_config = importlib.import_module("metaflow.metaflow_config") - s3_module = importlib.import_module("metaflow.plugins.datatools.s3.s3") - importlib.reload(metaflow_config) - importlib.reload(s3_module) - # Keep the package-level export in sync for tests that import S3 later. - s3_package = importlib.import_module("metaflow.plugins.datatools.s3") - s3_package.S3 = s3_module.S3 - return s3_module.S3 +def _run_s3_subprocess(cwd, env, expected_tempdir=None): + """Run an S3 configuration scenario in a fresh Python interpreter.""" + repo_root = str(Path(__file__).resolve().parents[2]) + env = env.copy() + env["PYTHONPATH"] = os.pathsep.join( + path for path in (repo_root, env.get("PYTHONPATH")) if path + ) + if expected_tempdir is not None: + env["TEST_EXPECTED_METAFLOW_TEMPDIR"] = str(expected_tempdir) + else: + env.pop("TEST_EXPECTED_METAFLOW_TEMPDIR", None) + + import metaflow + + s3_class_before = metaflow.S3 + result = subprocess.run( + [sys.executable, "-c", S3_SUBPROCESS_SCRIPT], + cwd=str(cwd), + env=env, + capture_output=True, + text=True, + check=False, + timeout=30, + ) + assert ( + result.returncode == 0 + ), "S3 subprocess failed with exit code %d\nstdout:\n%s\nstderr:\n%s" % ( + result.returncode, + result.stdout, + result.stderr, + ) + assert metaflow.S3 is s3_class_before -def test_s3_client_readonly_cwd(mocker, monkeypatch): +def test_s3_client_readonly_cwd(tmp_path): """Test that S3 client doesn't require writable CWD (issue #854).""" - _mock_s3_dependencies(mocker) - original_tempdir = os.environ.get("METAFLOW_TEMPDIR") + readonly_dir = tmp_path / "readonly" + readonly_dir.mkdir() + os.chmod(readonly_dir, 0o555) + env = os.environ.copy() + env.pop("METAFLOW_TEMPDIR", None) + env["METAFLOW_HOME"] = str(tmp_path / "empty_metaflow_home") try: - with monkeypatch.context() as env: - env.delenv("METAFLOW_TEMPDIR", raising=False) - S3 = _load_s3_class() - - # Create a temporary read-only directory - with tempfile.TemporaryDirectory() as tmpdir: - readonly_dir = os.path.join(tmpdir, "readonly") - os.makedirs(readonly_dir, mode=0o555) - - # Change to read-only directory - original_cwd = os.getcwd() - try: - os.chdir(readonly_dir) - - # This should not raise an exception about permissions - # The S3 client should use system temp dir, not CWD - s3_client = S3() - - # Verify that tmpdir was created in system temp, not CWD - assert s3_client._tmpdir is not None - # The temp dir should NOT be in the current (read-only) directory - assert not s3_client._tmpdir.startswith(readonly_dir) - # The temp dir should be in the system temp directory - system_temp = tempfile.gettempdir() - assert s3_client._tmpdir.startswith(system_temp) - - # Clean up - s3_client.close() - finally: - os.chdir(original_cwd) - # Make directory writable again so it can be deleted - os.chmod(readonly_dir, 0o755) - assert os.environ.get("METAFLOW_TEMPDIR") == original_tempdir + _run_s3_subprocess(readonly_dir, env) finally: - # Restore the S3 module's import-time default after the isolated test. - _load_s3_class() + # Make directory writable again so pytest can clean it up. + os.chmod(readonly_dir, 0o755) -def test_s3_client_respects_metaflow_tempdir(mocker, monkeypatch, tmp_path): +def test_s3_client_respects_metaflow_tempdir(tmp_path): """Test that METAFLOW_TEMPDIR overrides the system temporary directory.""" - _mock_s3_dependencies(mocker) configured_dir = tmp_path / "configured" configured_dir.mkdir() - original_tempdir = os.environ.get("METAFLOW_TEMPDIR") - try: - with monkeypatch.context() as env: - env.setenv("METAFLOW_TEMPDIR", str(configured_dir)) - S3 = _load_s3_class() - - s3_client = S3() - try: - assert s3_client._tmproot == str(configured_dir) - assert os.path.dirname(s3_client._tmpdir) == str(configured_dir) - finally: - s3_client.close() - assert os.environ.get("METAFLOW_TEMPDIR") == original_tempdir - finally: - # Restore the S3 module's import-time default after the isolated test. - _load_s3_class() + env = os.environ.copy() + env["METAFLOW_TEMPDIR"] = str(configured_dir) + env["METAFLOW_HOME"] = str(tmp_path / "empty_metaflow_home") + _run_s3_subprocess(tmp_path, env, configured_dir)