Fix documentation and the blocking bugs for local backend #249
Conversation
WalkthroughThe changes update several benchmark configurations and function interfaces. Four microbenchmark JSON files now include a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant generate_input
Caller->>generate_input: Call with (data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func)
generate_input-->>Caller: Returns generated input dictionary
sequenceDiagram
participant PerfCost
participant DeploymentClient
PerfCost->>DeploymentClient: update_function(function, benchmark, False, '')
DeploymentClient-->>PerfCost: Returns update result
Assessment against linked issues
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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: 2
🧹 Nitpick comments (1)
docs/usage.md (1)
180-180: Add language specification to code blockThe fenced code block at line 180 is missing a language specification, which is recommended for proper syntax highlighting.
-``` +```bash🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
180-180: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
benchmarks/000.microbenchmarks/010.sleep/config.json(1 hunks)benchmarks/000.microbenchmarks/010.sleep/input.py(1 hunks)benchmarks/000.microbenchmarks/020.network-benchmark/config.json(1 hunks)benchmarks/000.microbenchmarks/020.network-benchmark/input.py(1 hunks)benchmarks/000.microbenchmarks/030.clock-synchronization/config.json(1 hunks)benchmarks/000.microbenchmarks/030.clock-synchronization/input.py(1 hunks)benchmarks/000.microbenchmarks/040.server-reply/config.json(1 hunks)benchmarks/000.microbenchmarks/040.server-reply/input.py(1 hunks)docs/usage.md(2 hunks)sebs/benchmark.py(1 hunks)sebs/config.py(0 hunks)sebs/experiments/perf_cost.py(1 hunks)
💤 Files with no reviewable changes (1)
- sebs/config.py
🧰 Additional context used
🧬 Code Definitions (4)
benchmarks/000.microbenchmarks/010.sleep/input.py (4)
benchmarks/000.microbenchmarks/030.clock-synchronization/input.py (1)
generate_input(6-7)benchmarks/000.microbenchmarks/020.network-benchmark/input.py (1)
generate_input(6-7)benchmarks/000.microbenchmarks/040.server-reply/input.py (1)
generate_input(11-12)sebs/benchmark.py (1)
generate_input(783-794)
sebs/experiments/perf_cost.py (1)
sebs/faas/system.py (1)
update_function(215-234)
benchmarks/000.microbenchmarks/020.network-benchmark/input.py (4)
benchmarks/000.microbenchmarks/030.clock-synchronization/input.py (1)
generate_input(6-7)benchmarks/000.microbenchmarks/040.server-reply/input.py (1)
generate_input(11-12)benchmarks/000.microbenchmarks/010.sleep/input.py (1)
generate_input(11-12)sebs/benchmark.py (1)
generate_input(783-794)
benchmarks/000.microbenchmarks/030.clock-synchronization/input.py (4)
benchmarks/000.microbenchmarks/020.network-benchmark/input.py (1)
generate_input(6-7)benchmarks/000.microbenchmarks/040.server-reply/input.py (1)
generate_input(11-12)benchmarks/000.microbenchmarks/010.sleep/input.py (1)
generate_input(11-12)sebs/benchmark.py (1)
generate_input(783-794)
🪛 markdownlint-cli2 (0.17.2)
docs/usage.md
180-180: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (15)
benchmarks/000.microbenchmarks/040.server-reply/config.json (1)
4-5: Adds required "modules" field to the configuration structure.This change adds the previously missing "modules" field to the configuration, which is required by the BenchmarkConfig class as seen in benchmark.py. This fix addresses one of the PR objectives to ensure the local backend works correctly out-of-the-box.
benchmarks/000.microbenchmarks/010.sleep/config.json (1)
4-5: Adds required "modules" field to the configuration structure.This change adds the previously missing "modules" field to the configuration, which is required by the BenchmarkConfig class as seen in benchmark.py. This fix ensures proper functionality when deserializing the configuration.
benchmarks/000.microbenchmarks/030.clock-synchronization/config.json (1)
4-5: Adds required "modules" field to the configuration structure.The addition of the empty "modules" array ensures the configuration structure matches what's expected by the BenchmarkConfig deserializer. This consistency across benchmarks helps prevent runtime errors.
benchmarks/000.microbenchmarks/020.network-benchmark/config.json (1)
4-5: Adds required "modules" field to the configuration structure.Adding the empty "modules" array ensures this benchmark's configuration follows the same pattern as other benchmarks, providing consistency and preventing potential deserialization issues in the BenchmarkConfig class.
sebs/benchmark.py (1)
433-437: Simplifies Docker image naming by removing version component.This change removes the version segment from the Docker build image name, which streamlines the build process as mentioned in the PR objectives. By using a simpler naming convention, the system will likely be more maintainable and less prone to versioning-related issues.
benchmarks/000.microbenchmarks/010.sleep/input.py (1)
11-11: Function signature updated correctlyThe
generate_inputfunction signature has been updated to match the interface defined insebs/benchmark.py, which helps standardize the benchmark input generation across the codebase.benchmarks/000.microbenchmarks/040.server-reply/input.py (1)
11-11: Function signature updated correctlyThe
generate_inputfunction signature has been updated to match the interface defined insebs/benchmark.py, which helps standardize the benchmark input generation across the codebase.sebs/experiments/perf_cost.py (1)
86-86: Method signature update correctly implementedThe call to
update_functionhas been updated to match the new method signature by adding two additional parameters:Falseforcontainer_deploymentand an empty string forcontainer_uri. This change is consistent with the updated method signature in thesystem.pyfile and aligns with the PR objective to fix blocking bugs for local backend.docs/usage.md (7)
77-79: Storage start command updated correctlyThe command has been updated to use a configuration file instead of hardcoded port values, allowing for more flexible deployment. Using
allparameter suggests it now starts both object and NoSQL storage services as defined in the configuration.
87-89: Configuration update command enhancedThe command now correctly adds architecture information (
x64) to the configuration, which is a necessary parameter for local deployment. The output is also properly directed to a new configuration file.
94-133: Storage configuration structure properly updatedThe JSON structure now correctly includes both object storage (minio) and NoSQL storage (scylladb) sections, providing a comprehensive configuration template. This aligns with the updated command that uses a configuration file.
139-140: Local start command correctly updated with container removal optionThe command now includes the
--remove-containersoption, which is essential information for users who want to automatically clean up after testing. This change aligns with the note added at lines 185-186.
171-176: Function invocation command improved with dynamic valuesThe curl command now dynamically extracts the function URL and input data from the output JSON file using
jq, which is a significant improvement over hardcoded values. This makes the documentation more general and maintainable.
182-183: Storage stop command updated to match the start commandThe command now uses
allparameter to stop all storage services, which is consistent with the updated start command usingallparameter.
185-186: Helpful note about container removalThis addition provides important clarification about container lifecycle management, explaining that containers won't be automatically removed without the specific flag.
| def generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func): | ||
| return {'output-bucket': output_buckets[0]} |
There was a problem hiding this comment.
Fix variable reference in function implementation
The function signature has been updated, but the implementation still references output_buckets[0] which no longer exists as a parameter. This will cause a runtime error.
def generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func):
- return {'output-bucket': output_buckets[0]}
+ return {'output-bucket': output_paths[0]}📝 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 generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func): | |
| return {'output-bucket': output_buckets[0]} | |
| def generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func): | |
| return {'output-bucket': output_paths[0]} |
🧰 Tools
🪛 Ruff (0.8.2)
7-7: Undefined name output_buckets
(F821)
| def generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func): | ||
| return {'output-bucket': output_buckets[0]} |
There was a problem hiding this comment.
Fix variable reference in function implementation
The function signature has been updated, but the implementation still references output_buckets[0] which no longer exists as a parameter. This will cause a runtime error.
def generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func):
- return {'output-bucket': output_buckets[0]}
+ return {'output-bucket': output_paths[0]}📝 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 generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func): | |
| return {'output-bucket': output_buckets[0]} | |
| def generate_input(data_dir, size, benchmarks_bucket, input_paths, output_paths, upload_func, nosql_func): | |
| return {'output-bucket': output_paths[0]} |
🧰 Tools
🪛 Ruff (0.8.2)
7-7: Undefined name output_buckets
(F821)
| @@ -142,18 +168,22 @@ The output file `out_benchmark.json` will contain the information on containers | |||
|
|
|||
| In our example, we can use `curl` to invoke the function with provided input: | |||
|
|
|||
| ``` | |||
| curl 172.17.0.3:9000 --request POST --data '{"random_len": 10,"username": "testname"}' --header 'Content-Type: application/json' | |||
| ```bash | |||
| jq '.deployment.local.storage = input' config/example.json out_storage.json > config/local_deployment.json | ||
| ```bash | ||
| jq '.deployment.local.storage = input' config/example.json out_storage.json | \ | ||
| jq '.experiments.architecture = "x64"' > config/local_deployment.json |
There was a problem hiding this comment.
Better idea - please add architecture to local part of config/example.json?
There was a problem hiding this comment.
Currently the suite doesn't read architecture from deployment section (the local part) of the configuration. Would you like me to add a logic that architecture defined in deployment section overwriting architecture defined in experiments? Or another workaround is sepcifying architecture when starting the function containers: ./sebs.py local start ... --architecture=x64.
There was a problem hiding this comment.
We can use the CLI argument :)
| @@ -430,11 +430,10 @@ def install_dependencies(self, output_dir): | |||
| ) | |||
| else: | |||
| repo_name = self._system_config.docker_repository() | |||
| image_name = "build.{deployment}.{language}.{runtime}-{version}".format( | |||
| image_name = "build.{deployment}.{language}.{runtime}".format( | |||
There was a problem hiding this comment.
We added version tags recently, but I didn't manage to update the images in DockerHub. Please remove this change, and I will update images with new tags :)
There was a problem hiding this comment.
Sure!
However, would it be useful if the benchmark suite could fall back to images w/o version tags when images w/ version cannot be found on DockerHub? If so, I could implement it here.
| @@ -46,9 +46,6 @@ def supported_language_versions( | |||
| ) -> List[str]: | |||
| languages = self._system_config.get(deployment_name, {}).get("languages", {}) | |||
| base_images = languages.get(language_name, {}).get("base_images", {}) | |||
|
|
|||
| if deployment_name == "local": | |||
There was a problem hiding this comment.
Why is this change necessary?
There was a problem hiding this comment.
When bringing function container up, the benchmark suite will check if language-version pair is supported by the backend system (local in our case), which invokes this function to obtain the supported language-version pairs. However, currently this function returns empty set for local backend.
The reason is that in commit 52f80a1, the corresponding configuration of local system is now wrapped with a architecture key:
--- a/config/systems.json
+++ b/config/systems.json
@@ -17,11 +17,13 @@
"languages": {
"python": {
"base_images": {
- "3.7": "python:3.7-slim",
- "3.8": "python:3.8-slim",
- "3.9": "python:3.9-slim",
- "3.10": "python:3.10-slim",
- "3.11": "python:3.11-slim"
+ "x64": {
+ "3.7": "python:3.7-slim",
+ "3.8": "python:3.8-slim",
+ "3.9": "python:3.9-slim",
+ "3.10": "python:3.10-slim",
+ "3.11": "python:3.11-slim"
+ }
},
"images": [
"run",
@@ -43,10 +45,12 @@
},
"nodejs": {
"base_images": {
- "14": "node:14-slim",
- "16": "node:16-slim",
- "18": "node:18-slim",
- "20": "node:20-slim"
+ "x64": {
+ "14": "node:14-slim",
+ "16": "node:16-slim",
+ "18": "node:18-slim",
+ "20": "node:20-slim"
+ }
},
"images": [
"run",As a result, this if branch for local here is not relevant anymore.
There was a problem hiding this comment.
Hi @mcopik,
Thanks for the review! I'm happy to have the opportunity to contribute to this repository.
I've replied to some of your comments regarding the further development plans. Once you've had a chance to review them, I'll start refining my PR.
| jq '.deployment.local.storage = input' config/example.json out_storage.json > config/local_deployment.json | ||
| ```bash | ||
| jq '.deployment.local.storage = input' config/example.json out_storage.json | \ | ||
| jq '.experiments.architecture = "x64"' > config/local_deployment.json |
There was a problem hiding this comment.
Currently the suite doesn't read architecture from deployment section (the local part) of the configuration. Would you like me to add a logic that architecture defined in deployment section overwriting architecture defined in experiments? Or another workaround is sepcifying architecture when starting the function containers: ./sebs.py local start ... --architecture=x64.
| @@ -430,11 +430,10 @@ def install_dependencies(self, output_dir): | |||
| ) | |||
| else: | |||
| repo_name = self._system_config.docker_repository() | |||
| image_name = "build.{deployment}.{language}.{runtime}-{version}".format( | |||
| image_name = "build.{deployment}.{language}.{runtime}".format( | |||
There was a problem hiding this comment.
Sure!
However, would it be useful if the benchmark suite could fall back to images w/o version tags when images w/ version cannot be found on DockerHub? If so, I could implement it here.
| @@ -46,9 +46,6 @@ def supported_language_versions( | |||
| ) -> List[str]: | |||
| languages = self._system_config.get(deployment_name, {}).get("languages", {}) | |||
| base_images = languages.get(language_name, {}).get("base_images", {}) | |||
|
|
|||
| if deployment_name == "local": | |||
There was a problem hiding this comment.
When bringing function container up, the benchmark suite will check if language-version pair is supported by the backend system (local in our case), which invokes this function to obtain the supported language-version pairs. However, currently this function returns empty set for local backend.
The reason is that in commit 52f80a1, the corresponding configuration of local system is now wrapped with a architecture key:
--- a/config/systems.json
+++ b/config/systems.json
@@ -17,11 +17,13 @@
"languages": {
"python": {
"base_images": {
- "3.7": "python:3.7-slim",
- "3.8": "python:3.8-slim",
- "3.9": "python:3.9-slim",
- "3.10": "python:3.10-slim",
- "3.11": "python:3.11-slim"
+ "x64": {
+ "3.7": "python:3.7-slim",
+ "3.8": "python:3.8-slim",
+ "3.9": "python:3.9-slim",
+ "3.10": "python:3.10-slim",
+ "3.11": "python:3.11-slim"
+ }
},
"images": [
"run",
@@ -43,10 +45,12 @@
},
"nodejs": {
"base_images": {
- "14": "node:14-slim",
- "16": "node:16-slim",
- "18": "node:18-slim",
- "20": "node:20-slim"
+ "x64": {
+ "14": "node:14-slim",
+ "16": "node:16-slim",
+ "18": "node:18-slim",
+ "20": "node:20-slim"
+ }
},
"images": [
"run",As a result, this if branch for local here is not relevant anymore.
This PR introduces serveral mini fixes to make the local backend runnable out-of-the-box again.
basic_imagefield in the configuration for the local backend.update_functionof theLocalsystem.Closes #248.
Summary by CodeRabbit
New Features
Documentation
Refactor