fix: default TEMPDIR to system temp so S3 works with read-only CWD - #3372
fix: default TEMPDIR to system temp so S3 works with read-only CWD#3372Shriprasad-P wants to merge 1 commit into
Conversation
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 Netflix#854
Greptile SummaryThis PR changes the default Metaflow temporary directory from the current working directory to Python’s system temporary directory.
Confidence Score: 5/5The PR appears safe to merge, with only previously reported non-blocking documentation and test-isolation concerns remaining. No new issues were introduced since the previous review. The unresolved documentation thread remains outstanding because the public guidance was not updated, and the unresolved test thread remains outstanding because the assertion still depends on
|
| Filename | Overview |
|---|---|
| metaflow/metaflow_config.py | Changes the default temporary-directory base to the system temporary directory. |
| test/unit/test_s3_readonly_cwd.py | Adds coverage for constructing an S3 client while the current working directory is read-only. |
Reviews (2): Last reviewed commit: "Fix S3 client to use system temp directo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) | ||
|
|
||
| TEMPDIR = from_conf("TEMPDIR", ".") | ||
| TEMPDIR = from_conf("TEMPDIR", tempfile.gettempdir()) |
There was a problem hiding this comment.
Changing TEMPDIR to use the system temporary directory leaves the public S3 documentation and remote-runtime comments saying that tmproot and current.tempdir default to the current working directory. This gives users incorrect guidance about where temporary files are created. Please update the affected documentation alongside the new default.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| 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) |
There was a problem hiding this comment.
If METAFLOW_TEMPDIR is set, S3 uses that configured directory while this assertion still requires the path to start with tempfile.gettempdir(). The test then fails even though S3 behaves correctly. Please isolate this default-behavior test from the override or assert against the effective configured value.
88d4d64 to
6c18caf
Compare
1 participant
Fixes
Fixes #854
Summary
Default
TEMPDIRtotempfile.gettempdir()instead of., so the S3 client no longer requires the current working directory to be writable.Test plan