New CLI options #243
New CLI options #243
Conversation
WalkthroughThe changes introduce a new command in the benchmark group that enables packaging of experiments by configuring Docker image tag prefixes and determining function names. Additionally, logging details have been added to cache update methods for improved traceability. A new method in the deployment system now builds functions with language version verification and default naming. A standalone script for updating Docker images, which parses CLI arguments, reads JSON configuration, and selectively pulls images based on their SHA, has also been added. The existing process command remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as sebs.py (package command)
participant C as sebs_client
participant E as Experiment Config
participant D as Deployment Client (build_function)
U->>S: Invoke "package" command with benchmark, function_name, image_tag_prefix, kwargs
S->>C: Update configuration (apply image_tag_prefix if provided)
S->>E: Retrieve experiment configuration and update benchmark
E-->>S: Return benchmark data
S->>D: Call build_function (with benchmark object and function name)
D->>D: Check language version and determine default function name (if missing)
D->>D: Build function using package_code method
D-->>S: Return build result
S-->>U: Output command result
sequenceDiagram
participant U as User
participant T as update_docker_images.py
participant A as Argparse Parser
participant J as JSON Config
participant P as Pull Functions
U->>T: Execute script with CLI arguments
T->>A: Parse deployment type, image type, language, and version
T->>J: Read Docker repository configuration data
T->>P: Iterate over systems & invoke pull functions (generic_pull, pull_function, pull_language)
P->>P: Compare image SHA to decide update necessity
P-->>T: Return update status (updated or up-to-date)
T-->>U: Output final image update status
Possibly related PRs
Poem
✨ 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 (
|
![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 (4)
tools/update_docker_images.py (4)
1-23: Use context manager for file operations.The script should use a context manager when opening files to ensure they are properly closed even if an exception occurs.
-config = json.load(open(os.path.join(PROJECT_DIR, "config", "systems.json"), "r")) +with open(os.path.join(PROJECT_DIR, "config", "systems.json"), "r") as f: + config = json.load(f)🧰 Tools
🪛 Ruff (0.8.2)
20-20: Use a context manager for opening files
(SIM115)
82-85: Simplify conditional logic.The conditional statements for checking language version can be simplified using the logical OR operator.
- if args.language_version is not None and args.language_version == version: - configs.append([version, base_image]) - elif args.language_version is None: - configs.append([version, base_image]) + if args.language_version is None or args.language_version == version: + configs.append([version, base_image])🧰 Tools
🪛 Ruff (0.8.2)
82-85: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
114-118: Unused loop variable in nested loop.The
image_configvariable is not used within the loop body.- for image_type, image_config in system_config["images"].items(): + for image_type, _ in system_config["images"].items():🧰 Tools
🪛 Ruff (0.8.2)
116-116: Loop control variable
image_confignot used within loop bodyRename unused
image_configto_image_config(B007)
53-70: Consider making benchmarks configuration more maintainable.The hardcoded benchmark list could become difficult to maintain as more benchmarks are added. Consider loading this from a configuration file or creating a function to discover available benchmarks.
For example, you could load this from a JSON configuration file or use a directory structure to discover benchmarks automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sebs.py(1 hunks)sebs/cache.py(2 hunks)sebs/faas/system.py(1 hunks)tools/update_docker_images.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tools/update_docker_images.py
20-20: Use a context manager for opening files
(SIM115)
82-85: Combine if branches using logical or operator
Combine if branches
(SIM114)
116-116: Loop control variable image_config not used within loop body
Rename unused image_config to _image_config
(B007)
sebs.py
351-351: Local variable func is assigned to but never used
Remove assignment to unused variable func
(F841)
🔇 Additional comments (5)
sebs/cache.py (2)
225-226: Good addition of logging information.Adding this log statement improves traceability by providing visibility into code package updates. This will help with debugging cache operations.
271-271: Good consistency in logging.This log message maintains consistency with the one added in
add_code_packagemethod, providing clear information about cache operations.sebs.py (1)
310-355: The newpackagecommand is well-structured.The implementation follows the same pattern as other commands and reuses the common parameter handling logic, which is good for consistency.
🧰 Tools
🪛 Ruff (0.8.2)
351-351: Local variable
funcis assigned to but never usedRemove assignment to unused variable
func(F841)
sebs/faas/system.py (1)
185-201: Good addition of thebuild_functionmethod.This method extracts common functionality from
get_function, promoting code reuse. The implementation includes proper error handling and makes good use of defaults.However, there's one issue to consider: unlike
get_functionwhich returns aFunctionobject, this method doesn't return anything but assigns a value to a variable. This inconsistency might lead to confusion.def build_function(self, code_package: Benchmark, func_name: Optional[str] = None): + """ + Build a function from the given code package without creating it in the FaaS system. + + :param code_package: The benchmark code package + :param func_name: Optional name for the function, defaults to system-generated name + :return: The built function object + """ if code_package.language_version not in self.system_config.supported_language_versions( self.name(), code_package.language_name ): raise Exception( "Unsupported {language} version {version} in {system}!".format( language=code_package.language_name, version=code_package.language_version, system=self.name(), ) ) if not func_name: func_name = self.default_function_name(code_package) - code_package.build(self.package_code) + rebuilt, _ = code_package.build(self.package_code) + return func_nametools/update_docker_images.py (1)
24-42: Good implementation of the image pulling function.The
pullfunction correctly handles cases where an image doesn't exist and provides clear feedback about image updates. The error handling is appropriate for this use case.
…marks into feature/better-cli
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sebs.py (1)
337-381:⚠️ Potential issueFix unused variable
funcThe function builds the benchmark but doesn't do anything with the returned function object. This is the same issue as mentioned in a previous review comment, which was addressed in another part of the code.
Apply this diff to fix the issue:
- func = deployment_client.build_function( + deployment_client.build_function( benchmark_obj, function_name if function_name else deployment_client.default_function_name(benchmark_obj) )Or if you need to inform the user about the built function:
func = deployment_client.build_function( benchmark_obj, function_name if function_name else deployment_client.default_function_name(benchmark_obj) ) + sebs_client.logging.info(f"Successfully built function: {func.name}")🧰 Tools
🪛 Ruff (0.8.2)
378-378: Local variable
funcis assigned to but never usedRemove assignment to unused variable
func(F841)
🧹 Nitpick comments (4)
sebs/storage/scylladb.py (1)
96-96: Consider the implications of changing container removal behaviorThe PR changes the container removal behavior from always removing containers (
remove=True) to making it configurable viaself.config.remove_containers. Since the default value inScyllaDBConfigisFalse, this represents a behavior change that could lead to container accumulation if users aren't explicitly cleaning up.Consider adding documentation about this behavior change and potentially logging a message when containers aren't being automatically removed. This would help users understand why containers might persist after stopping.
sebs/storage/config.py (1)
33-33: Add documentation for the new configuration optionThe new
remove_containersattribute has been added to bothMinioConfigandScyllaDBConfigclasses with a default value ofFalse. This is a behavior change from the previous implicitTruevalue.Add docstrings to explain what this configuration option does, especially noting that the default behavior is to keep containers (different from previous versions):
@dataclass class MinioConfig(PersistentStorageConfig): + """Configuration for Minio storage. + + Attributes: + remove_containers: Whether to automatically remove containers when stopped. + Defaults to False to allow post-mortem inspection. + """And similarly for
ScyllaDBConfig.Also applies to: 83-83
sebs/storage/minio.py (1)
103-103: Aligned container removal behavior with ScyllaDBGood consistency in changing both storage implementations to use the same configuration-based approach for container removal.
Consider adding a log message when a container isn't automatically removed, similar to the suggestion for ScyllaDB:
self._storage_container = self._docker_client.containers.run( # ...existing parameters remove=self.config.remove_containers, # ...remaining parameters ) +if not self.config.remove_containers: + self.logging.info(f"Container {self._storage_container.id} will not be automatically removed after stopping")sebs/faas/system.py (1)
236-252: Missing method documentationThe new
build_functionmethod lacks documentation, making it unclear what it does and how it differs fromget_function.Add docstrings to explain the purpose and behavior of this method:
def build_function(self, code_package: Benchmark, func_name: Optional[str] = None): + """ + Builds a function package without creating/updating it in the cloud. + + This is useful for packaging experiments without deploying them. + + Args: + code_package: Benchmark containing the function code + func_name: Optional name for the function, defaults to generated name + + Returns: + Tuple[bool, str, bool, str]: Build results containing: + - whether it was rebuilt + - the code location + - whether it's a container deployment + - the container URI + + Raises: + Exception: If the language version is not supported + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
sebs.py(4 hunks)sebs/cache.py(2 hunks)sebs/faas/system.py(1 hunks)sebs/storage/config.py(2 hunks)sebs/storage/minio.py(1 hunks)sebs/storage/scylladb.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sebs/cache.py
🧰 Additional context used
🧬 Code Definitions (3)
sebs/storage/config.py (1)
sebs/local/local.py (2)
remove_containers(44-45)remove_containers(48-49)
sebs.py (3)
sebs/utils.py (2)
default(23-32)update_nested_dict(56-61)sebs/config.py (2)
image_tag_prefix(14-15)image_tag_prefix(18-19)sebs/sebs.py (5)
config(44-45)output_dir(32-33)logging_filename(40-41)get_experiment_config(152-153)get_benchmark(180-199)
sebs/faas/system.py (3)
sebs/benchmark.py (6)
code_package(102-103)Benchmark(84-762)language_version(150-151)language_name(146-147)language(142-143)build(561-625)sebs/config.py (2)
supported_language_versions(44-52)version(70-71)sebs/faas/function.py (1)
name(365-366)
🪛 Ruff (0.8.2)
sebs.py
378-378: Local variable func is assigned to but never used
Remove assignment to unused variable func
(F841)
🔇 Additional comments (2)
sebs.py (2)
438-443: LGTM!The addition of the
--remove-containers/--no-remove-containersoption provides useful control over container cleanup behavior. This change follows the existing pattern for command-line options in the codebase.
458-458: LGTM!The implementation correctly passes the
remove_containersparameter to both storage configuration types.Also applies to: 477-477
| def build_function(self, code_package: Benchmark, func_name: Optional[str] = None): | ||
|
|
||
| if code_package.language_version not in self.system_config.supported_language_versions( | ||
| self.name(), code_package.language_name | ||
| ): | ||
| raise Exception( | ||
| "Unsupported {language} version {version} in {system}!".format( | ||
| language=code_package.language_name, | ||
| version=code_package.language_version, | ||
| system=self.name(), | ||
| ) | ||
| ) | ||
|
|
||
| if not func_name: | ||
| func_name = self.default_function_name(code_package) | ||
| code_package.build(self.package_code) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing return value for build result
The method calls code_package.build(self.package_code) which returns a tuple with build results, but build_function doesn't return any value to the caller. Looking at the PR objectives, this method is likely used to build without uploading, so returning build information would be helpful.
Return the build result to provide information to the caller:
def build_function(self, code_package: Benchmark, func_name: Optional[str] = None):
if code_package.language_version not in self.system_config.supported_language_versions(
self.name(), code_package.language_name
):
# ...exception handling
if not func_name:
func_name = self.default_function_name(code_package)
- code_package.build(self.package_code)
+ return code_package.build(self.package_code)📝 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.
| def build_function(self, code_package: Benchmark, func_name: Optional[str] = None): | |
| if code_package.language_version not in self.system_config.supported_language_versions( | |
| self.name(), code_package.language_name | |
| ): | |
| raise Exception( | |
| "Unsupported {language} version {version} in {system}!".format( | |
| language=code_package.language_name, | |
| version=code_package.language_version, | |
| system=self.name(), | |
| ) | |
| ) | |
| if not func_name: | |
| func_name = self.default_function_name(code_package) | |
| code_package.build(self.package_code) | |
| def build_function(self, code_package: Benchmark, func_name: Optional[str] = None): | |
| if code_package.language_version not in self.system_config.supported_language_versions( | |
| self.name(), code_package.language_name | |
| ): | |
| raise Exception( | |
| "Unsupported {language} version {version} in {system}!".format( | |
| language=code_package.language_name, | |
| version=code_package.language_version, | |
| system=self.name(), | |
| ) | |
| ) | |
| if not func_name: | |
| func_name = self.default_function_name(code_package) | |
| return code_package.build(self.package_code) |
Architecture parameter missing in supported_language_versions check
The implementation of build_function is missing the architecture parameter when checking for supported language versions, unlike the existing get_function method.
This could lead to inconsistent behavior between the two methods. The supported_language_versions method in sebs/config.py expects an architecture parameter and uses it to filter the available base images.
Apply this fix:
def build_function(self, code_package: Benchmark, func_name: Optional[str] = None):
if code_package.language_version not in self.system_config.supported_language_versions(
- self.name(), code_package.language_name
+ self.name(), code_package.language_name, code_package.architecture
):
raise Exception(📝 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.
| def build_function(self, code_package: Benchmark, func_name: Optional[str] = None): | |
| if code_package.language_version not in self.system_config.supported_language_versions( | |
| self.name(), code_package.language_name | |
| ): | |
| raise Exception( | |
| "Unsupported {language} version {version} in {system}!".format( | |
| language=code_package.language_name, | |
| version=code_package.language_version, | |
| system=self.name(), | |
| ) | |
| ) | |
| if not func_name: | |
| func_name = self.default_function_name(code_package) | |
| code_package.build(self.package_code) | |
| def build_function(self, code_package: Benchmark, func_name: Optional[str] = None): | |
| if code_package.language_version not in self.system_config.supported_language_versions( | |
| self.name(), code_package.language_name, code_package.architecture | |
| ): | |
| raise Exception( | |
| "Unsupported {language} version {version} in {system}!".format( | |
| language=code_package.language_name, | |
| version=code_package.language_version, | |
| system=self.name(), | |
| ) | |
| ) | |
| if not func_name: | |
| func_name = self.default_function_name(code_package) | |
| code_package.build(self.package_code) |
We added a script for updating Docker images, and a new CLI option to create build package without uploading.
Missing tasks
Summary by CodeRabbit
New Features
Chores