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/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 new file mode 100644 index 00000000000..44e6d1e7c55 --- /dev/null +++ b/test/unit/test_s3_readonly_cwd.py @@ -0,0 +1,117 @@ +"""Test that S3 client works when current directory is not writeable.""" + +import os +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 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 + + 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 _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(tmp_path): + """Test that S3 client doesn't require writable CWD (issue #854).""" + 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: + _run_s3_subprocess(readonly_dir, env) + finally: + # Make directory writable again so pytest can clean it up. + os.chmod(readonly_dir, 0o755) + + +def test_s3_client_respects_metaflow_tempdir(tmp_path): + """Test that METAFLOW_TEMPDIR overrides the system temporary directory.""" + configured_dir = tmp_path / "configured" + configured_dir.mkdir() + + 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)