Change temporary directory path to './tmp' #640
Conversation
Change temporary directory path to './tmp' to avoid space issue on different partition
WalkthroughThis change updates storage path defaults from absolute Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tplr/comms.py(6 hunks)
🔇 Additional comments (1)
src/tplr/comms.py (1)
120-122: Unrelated formatting change to type annotation.The type annotation for
_s3_clientshas been reformatted across multiple lines. This change appears unrelated to the PR objective of changing temporary directory paths.Is this formatting change intentional, or was it accidentally introduced by an IDE auto-formatter? If intentional, consider mentioning it in the PR description for clarity.
| # Constants | ||
| CF_REGION_NAME: str = "enam" | ||
| LOCAL_TMP_DIR = "/tmp/local_store" | ||
| LOCAL_TMP_DIR = "./tmp/local_store" |
There was a problem hiding this comment.
Relative path creates working-directory dependency and potential data fragmentation.
Using "./tmp/local_store" instead of an absolute path means the storage location depends on the current working directory at runtime. This introduces several risks:
- Data fragmentation: Multiple instances or runs from different directories will create separate tmp directories, leading to data inconsistency.
- Cleanup issues: Stale data cleanup becomes unreliable when files are scattered across multiple locations.
- Cross-partition: The PR objective of avoiding cross-partition issues may not be achieved—it depends on where the working directory is located.
Consider one of these solutions:
Solution 1 (Recommended): Use an absolute path with a configurable base directory:
+import pathlib
+
+# Allow configuration via environment variable, default to project root
+_BASE_DIR = os.getenv("TPLR_STORAGE_BASE", str(pathlib.Path(__file__).parent.parent.parent))
-LOCAL_TMP_DIR = "./tmp/local_store"
+LOCAL_TMP_DIR = os.path.join(_BASE_DIR, "tmp", "local_store")Solution 2: Resolve to absolute path immediately:
-LOCAL_TMP_DIR = "./tmp/local_store"
+LOCAL_TMP_DIR = os.path.abspath("./tmp/local_store")Solution 3: If space is the concern, use a dedicated disk mount:
-LOCAL_TMP_DIR = "./tmp/local_store"
+LOCAL_TMP_DIR = os.getenv("TPLR_TMP_DIR", "/var/lib/templar/local_store")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOCAL_TMP_DIR = "./tmp/local_store" | |
| LOCAL_TMP_DIR = os.path.abspath("./tmp/local_store") |
| LOCAL_TMP_DIR = "./tmp/local_store" | |
| LOCAL_TMP_DIR = os.getenv("TPLR_TMP_DIR", "/var/lib/templar/local_store") |
🤖 Prompt for AI Agents
In src/tplr/comms.py around line 52, the LOCAL_TMP_DIR is a relative path
("./tmp/local_store") which ties storage to the current working directory and
risks fragmented/stale data and cross-partition issues; change it to an
absolute, configurable path (e.g., derive from an application config or
environment variable with a sensible default like the system temp dir or a
dedicated base directory), resolve it to an absolute path at module load time,
and ensure the directory is created with correct permissions so all runs use a
single well-known location; update any callers to use the resolved path rather
than the original relative constant.
| self, | ||
| wallet: bt.wallet | None, | ||
| save_location: str = "/tmp", | ||
| save_location: str = "./tmp", |
There was a problem hiding this comment.
Default parameter change introduces backward compatibility risk.
Changing the default from "/tmp" to "./tmp" could break existing deployments or scripts that expect files in /tmp. Additionally, the relative path inherits all the issues flagged for LOCAL_TMP_DIR.
Apply this diff to use a resolved absolute path:
+import pathlib
+
+# Constant for default storage base
+_DEFAULT_STORAGE = os.path.abspath(os.getenv("TPLR_STORAGE_BASE", "./tmp"))
+
def __init__(
self,
wallet: bt.wallet | None,
- save_location: str = "./tmp",
+ save_location: str = _DEFAULT_STORAGE,
key_prefix: str = "model",
config=None,
hparams=None,
uid: int | None = None,
**kwargs,
):Also update the docstring:
- save_location (str, optional): The base directory for saving local files. Defaults to "./tmp".
+ save_location (str, optional): The base directory for saving local files. Defaults to resolved absolute path from TPLR_STORAGE_BASE or "./tmp".Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/tplr/comms.py around line 75, the default save_location was changed from
"/tmp" to "./tmp", which risks breaking deployments and inherits relative-path
issues; revert to an absolute resolved path by setting the default to a resolved
absolute temp directory (e.g., resolve the configured LOCAL_TMP_DIR or use
pathlib.Path("/tmp").resolve()) and ensure the function normalizes/creates the
directory before use; also update the function docstring to document that
save_location is an absolute path, how it is resolved, and that the directory
will be created if missing.
| self.temp_dir = os.path.join("./tmp", f"templar_{self.uid}") | ||
| os.makedirs(self.temp_dir, exist_ok=True) |
There was a problem hiding this comment.
Hardcoded path bypasses the save_location parameter.
The temp_dir is constructed using a hardcoded "./tmp" prefix instead of the save_location parameter that was just passed to __init__. This creates an inconsistency where the user's specified save_location is ignored for temp directory creation.
Apply this diff to respect the save_location parameter:
# Create temp directory for this instance
- self.temp_dir = os.path.join("./tmp", f"templar_{self.uid}")
+ self.temp_dir = os.path.join(os.path.abspath(save_location), f"templar_{self.uid}")
os.makedirs(self.temp_dir, exist_ok=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.temp_dir = os.path.join("./tmp", f"templar_{self.uid}") | |
| os.makedirs(self.temp_dir, exist_ok=True) | |
| self.temp_dir = os.path.join(os.path.abspath(save_location), f"templar_{self.uid}") | |
| os.makedirs(self.temp_dir, exist_ok=True) |
🤖 Prompt for AI Agents
In src/tplr/comms.py around lines 99 to 100, the temp_dir is created with a
hardcoded "./tmp" prefix which ignores the save_location parameter passed to
__init__; change the construction to join with the instance's save_location
(e.g., os.path.join(self.save_location, "tmp", f"templar_{self.uid}") or
os.path.join(self.save_location, f"templar_{self.uid}") depending on desired
structure) and keep os.makedirs(self.temp_dir, exist_ok=True) so the temp
directory is created under the provided save_location.
| self.save_location = os.path.join("./tmp", f"hotkey_{hotkey}") | ||
| os.makedirs(self.save_location, exist_ok=True) |
There was a problem hiding this comment.
The save_location parameter is overridden and ignored.
Line 114 unconditionally overwrites self.save_location with a hardcoded "./tmp" prefix, ignoring the value passed by the caller in the save_location parameter. This makes the parameter misleading.
Apply this diff to respect the parameter:
# Use the hotkey directly in the save_location
if self.wallet is not None:
hotkey = self.wallet.hotkey.ss58_address
- self.save_location = os.path.join("./tmp", f"hotkey_{hotkey}")
+ self.save_location = os.path.join(os.path.abspath(save_location), f"hotkey_{hotkey}")
os.makedirs(self.save_location, exist_ok=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.save_location = os.path.join("./tmp", f"hotkey_{hotkey}") | |
| os.makedirs(self.save_location, exist_ok=True) | |
| self.save_location = os.path.join(os.path.abspath(save_location), f"hotkey_{hotkey}") | |
| os.makedirs(self.save_location, exist_ok=True) |
🤖 Prompt for AI Agents
In src/tplr/comms.py around lines 114-115, the code currently overwrites the
caller-provided save_location with a hardcoded "./tmp" prefix; instead use the
incoming save_location value (or default) as the base directory when
constructing self.save_location so the parameter is respected: set base =
save_location if provided (fall back to "./tmp" only when None/empty), then do
self.save_location = os.path.join(base, f"hotkey_{hotkey}") and call
os.makedirs(self.save_location, exist_ok=True); ensure you do not
unconditionally assign "./tmp" before this logic.
| temp_dir = os.path.join("./tmp", str(self.uid)) | ||
| os.makedirs(temp_dir, exist_ok=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicates temp directory creation instead of reusing self.temp_dir.
The put() method creates its own temp directory with os.path.join("./tmp", str(self.uid)) instead of reusing self.temp_dir that was already created in __init__. This creates potential for inconsistency and file fragmentation.
Apply this diff to reuse the existing temp directory:
put_start = tplr.T()
- # Create per-uid temp directory
- temp_dir = os.path.join("./tmp", str(self.uid))
- os.makedirs(temp_dir, exist_ok=True)
+ # Use the instance's temp directory
+ temp_dir = self.temp_dir
temp_file_path = os.path.join(temp_dir, f"temp_{filename}")This ensures all temporary files for an instance are stored in a single location, simplifying cleanup and improving consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| temp_dir = os.path.join("./tmp", str(self.uid)) | |
| os.makedirs(temp_dir, exist_ok=True) | |
| # Use the instance's temp directory | |
| temp_dir = self.temp_dir |
🤖 Prompt for AI Agents
In src/tplr/comms.py around lines 1302 to 1303, the put() method creates a new
temp_dir with os.path.join("./tmp", str(self.uid)) instead of reusing the
instance's existing self.temp_dir; change the code to use self.temp_dir (which
was created in __init__) for all temporary file operations so all temp files
live in the same directory and avoid fragmentation—replace the local temp_dir
creation with a reference to self.temp_dir and ensure os.makedirs is called only
if needed or removed if already created in __init__.
1 participant
Change temporary directory path to './tmp' to avoid space issue on different partition
Description
Related Issue(s)
Type of Change
Branch Naming
Commit Messages
Code Quality
Testing
Documentation
If this is a breaking change
Screenshots/Examples
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes