added script to build docker/singularity images #703
Conversation
WalkthroughThe pull request introduces two major changes. First, an additional entry ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script
participant ArgumentParser
participant Cleaner
participant FrameworkRunner
User->>Script: Execute build_images.py
Script->>ArgumentParser: Parse command-line arguments
ArgumentParser-->>Script: Return mode, flags, and frameworks
alt Docker mode with --force
Script->>Cleaner: Delete Docker images for specified frameworks
else Singularity mode with --force
Script->>Cleaner: Delete Singularity files (Singularityfile, .sif)
end
Script->>FrameworkRunner: Run benchmark for each framework
FrameworkRunner-->>Script: Benchmark execution complete
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #703 +/- ##
=======================================
Coverage 69.53% 69.53%
=======================================
Files 57 57
Lines 6831 6831
=======================================
Hits 4750 4750
Misses 2081 2081 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
scripts/build_images.py (6)
1-16: Documentation looks good, but could use execution context.The documentation clearly explains how to use the script with good examples. Consider adding a note about the expected working directory for running the script.
""" This script can be used to build all/selected docker/singularity images. 0. Load your virtual environment like you would when using AMLB. +0a. Navigate to the 'scripts' directory before running this script. 1. Run the script with the required mode: python build_images.py -m docker python build_images.py -m singularity
18-23: Remove redundant import.You're importing both
pathlibandPathfrompathlib, which is redundant.import os import argparse -import pathlib from pathlib import Path from typing import List, OptionalThen update the type annotation in
delete_singularity_filesfunction accordingly.
25-44: Ensure consistent case handling in framework matching.The function lowers the case of the file name but not the frameworks when checking if a framework is in the file name. While frameworks are lowercased when created in
get_frameworks, there's no guarantee they'll be passed that way to this function.def delete_singularity_files( - directory: pathlib.Path, frameworks: Optional[List[str]] = None + directory: Path, frameworks: Optional[List[str]] = None ) -> None: """Deletes Singularity-related files from the specified directory.""" for root, _, files in os.walk(directory): for file in files: if file == "Singularityfile" or file.endswith(".sif"): file_path = Path(root) / file if frameworks and not any( - framework in file_path.name.lower() for framework in frameworks + framework.lower() in file_path.name.lower() for framework in frameworks ): continue
46-63: Add validation for mode argument.The parser should validate that the mode is either "docker" or "singularity" to prevent errors later in the script.
parser.add_argument( - "-m", "--mode", help="Docker or singularity", type=str, required=True + "-m", "--mode", + help="Docker or singularity", + type=str, + required=True, + choices=["docker", "singularity"] )
66-78: Validate user-specified frameworks.The function doesn't check if user-specified frameworks exist in the framework directory, which could lead to errors later.
def get_frameworks(framework_dir: Path, user_input: Optional[str]) -> List[str]: """Gets the list of frameworks based on user input or directory listing.""" + available_frameworks = [ + framework.lower() + for framework in os.listdir(framework_dir) + if "__" not in framework + ] + if user_input: frameworks = [framework.lower().strip() for framework in user_input.split(",")] + # Validate user-specified frameworks + invalid_frameworks = [f for f in frameworks if f not in available_frameworks] + if invalid_frameworks: + print(f"Warning: The following frameworks were not found: {invalid_frameworks}") + frameworks = [f for f in frameworks if f in available_frameworks] + if not frameworks: + print("No valid frameworks specified. Exiting.") + exit(1) print(f"Running for given frameworks - {frameworks}") else: - frameworks = [ - framework.lower() - for framework in os.listdir(framework_dir) - if "__" not in framework - ] + frameworks = available_frameworks print(f"Running for all frameworks - {frameworks}") return frameworks
83-83: Use absolute paths for better reliability.The script uses relative paths (
../frameworks/and../runbenchmark.py), which means it must be run from a specific location. Using absolute paths would make the script more robust.def main() -> None: args = parse_arguments() - framework_dir = Path("../frameworks/") + # Get the absolute path of the script + script_dir = Path(__file__).parent.absolute() + # Get the absolute path of the project root (parent of scripts directory) + project_root = script_dir.parent + framework_dir = project_root / "frameworks"Then update the path to
runbenchmark.pyin the subprocess call:subprocess.run( - ["python", "../runbenchmark.py", framework, "openml/t/3812", "--mode", mode, "--setup", "only"], + ["python", str(project_root / "runbenchmark.py"), framework, "openml/t/3812", "--mode", mode, "--setup", "only"], input=b"yes\n" * 10, check=True )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)scripts/build_images.py(1 hunks)
🔇 Additional comments (1)
.gitignore (1)
14-14: LGTM: Good addition to .gitignore.Adding
.venv/is a sensible enhancement that complements the existingvenv/entry, ensuring Git ignores virtual environments regardless of the naming convention used.
2 participants
This script can be used to build all/selected docker/singularity images. You can either recreate them or just rebuild. Works for both singularity and docker (atleast for me)
Summary by CodeRabbit
New Features
Chores