Add versioning and resource_id to function names(#230) #232
Conversation
WalkthroughThis pull request introduces a new version tracking mechanism for the Serverless Benchmarking System (SeBS). Changes include the addition of a Changes
Sequence DiagramsequenceDiagram
participant Config as Config System
participant BuildTool as Build Docker Images
participant Deployment as Deployment Client
Config->>Config: Add SeBS_version
Config->>BuildTool: Pass SeBS version
BuildTool->>BuildTool: Tag images with version
Deployment->>Deployment: Generate function names with resources
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (2)
config/systems.json (1)
3-4: Consider using environment variables for versioning.While adding versioning is good, hardcoding the version in the config file may lead to version drift. Consider:
- Using environment variables for the version.
- Adding a VERSION file at the root of the project.
- Using git tags for version tracking.
This would make version management more maintainable and automated.
sebs/gcp/gcp.py (1)
107-111: LGTM! Consider these minor improvements.The changes align well with the PR objectives. Two suggestions to improve the code:
- Consider using named placeholders for better readability
- Consider converting this to an instance method since it uses an instance parameter
- @staticmethod - def default_function_name(code_package: Benchmark,resources:Resources) -> str: - # Create function name - func_name = "{}-{}-{}-{}".format( - code_package.benchmark, code_package.language_name, code_package.language_version,resources.resources_id - ) + def default_function_name(self, code_package: Benchmark, resources: Resources) -> str: + # Create function name + func_name = "{benchmark}-{lang}-{ver}-{res_id}".format( + benchmark=code_package.benchmark, + lang=code_package.language_name, + ver=code_package.language_version, + res_id=resources.resources_id + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
config/systems.json(1 hunks)sebs.py(1 hunks)sebs/aws/aws.py(1 hunks)sebs/config.py(1 hunks)sebs/gcp/gcp.py(1 hunks)sebs/local/local.py(2 hunks)sebs/openwhisk/openwhisk.py(2 hunks)tools/build_docker_images.py(1 hunks)
🔇 Additional comments (8)
sebs/config.py (1)
93-95: LGTM! Good use of dict.get() with default value.The implementation safely handles missing version information by defaulting to "unknown".
tools/build_docker_images.py (1)
40-41: LGTM! Consistent with config.py implementation.The implementation maintains consistency with config.py by using the same default value and version retrieval method.
sebs/local/local.py (2)
20-20: LGTM!The import of
Resourcesfromsebs.faas.configis correctly added to support the new resource ID functionality.
345-349: LGTM! Function name generation enhanced with resource ID.The function name format now includes the resource ID, which improves uniqueness and traceability of deployed functions.
sebs/openwhisk/openwhisk.py (2)
15-15: LGTM!The import of
Resourcesfromsebs.faas.configis correctly added to support the new resource ID functionality.
Line range hint
392-399: LGTM! Function name generation enhanced with resource ID.The function name format now includes the resource ID, which improves uniqueness and traceability of deployed functions.
sebs.py (1)
496-496: LGTM! Function call updated to include resource configuration.The call to
default_function_nameis correctly updated to pass the required resource configuration.sebs/aws/aws.py (1)
392-401: LGTM! Function name generation enhanced with resource ID and AWS-specific formatting.The function name format now includes the resource ID and maintains AWS Lambda naming restrictions by replacing hyphens and dots with underscores.
|
hi @mcopik please review this PR |
|
Very nice work! Almost ready to merge :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sebs/faas/system.py (1)
379-381: Add docstring to document the new parameter.The method lacks documentation for the new
resourcesparameter. Add a docstring to explain its purpose and usage.@abstractmethod def default_function_name( self, code_package: Benchmark, resources: Optional[Resources] = None ) -> str: + """ + Generate a default name for the function. + + Args: + code_package: Benchmark containing the function code + resources: Optional resources configuration containing resource_id + + Returns: + str: Generated function name + """ passsebs/azure/azure.py (2)
192-193: Consider implementing other trigger types.The TODO comment indicates that only HTTP triggers are supported. Consider implementing other common trigger types like timer, queue, or blob triggers to enhance the functionality.
Would you like me to help implement support for additional trigger types? I can create a new issue to track this enhancement.
520-520: Consider implementing memory metrics collection.The TODO comment indicates missing memory metrics collection. This could be valuable for benchmarking purposes.
Would you like me to help implement the memory metrics collection using Azure Monitor metrics API? I can create a new issue to track this enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
sebs/aws/aws.py(1 hunks)sebs/azure/azure.py(2 hunks)sebs/config.py(2 hunks)sebs/faas/system.py(1 hunks)sebs/gcp/gcp.py(1 hunks)sebs/local/local.py(2 hunks)sebs/openwhisk/openwhisk.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- sebs/aws/aws.py
- sebs/config.py
- sebs/local/local.py
- sebs/openwhisk/openwhisk.py
- sebs/gcp/gcp.py
🔇 Additional comments (5)
sebs/faas/system.py (2)
379-381: Method signature change requires updates to all implementing classes.The addition of the optional
resourcesparameter todefault_function_namerequires updates to all concrete subclasses implementing this abstract method. This change aligns with the PR objective of incorporating resource_id in function names for better identification.Let's verify that all implementing classes have been updated accordingly:
254-269: Verify the impact on existing function name generation.The
get_functionmethod callsdefault_function_namewithout providing theresourcesparameter. This needs to be verified to ensure it doesn't affect existing functionality.Let's check if there are any other callers of this method that might need updating:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find all calls to default_function_name to ensure they're updated # with the new parameter if needed. ast-grep --pattern 'default_function_name($$$)'Length of output: 49
🏁 Script executed:
#!/bin/bash # Let's search for the definition of default_function_name to inspect its signature. echo "Searching for default_function_name definitions:" rg "def default_function_name" -n . # Now, let's search for all calls to default_function_name to see their usage. echo "Searching for calls to default_function_name:" rg "default_function_name(" -n .Length of output: 811
🏁 Script executed:
#!/bin/bash # Show the definition of default_function_name in sebs/faas/system.py to inspect its parameters. sed -n '375,400p' sebs/faas/system.pyLength of output: 680
The default_function_name call in get_function is safe as written.
- The method’s signature defines
resourcesas an optional parameter with a default value of None, so omitting it has no impact on functionality.- No other call sites indicate that the
resourcesparameter is required.sebs/azure/azure.py (3)
22-22: LGTM! Import added for new Resources type.The import of
Resourcesfromsebs.faas.configaligns with the PR objectives of adding resource_id support.
326-328: LGTM! Method signature updated to support resource identification.The addition of the optional
resourcesparameter maintains backward compatibility while enabling resource-specific function naming.
332-342: Verify resource_id usage in function naming.The function name generation includes
self.config.resources.resources_idwhich aligns with the PR objectives. However, the providedresourcesparameter is not being used.Let's verify if this is intentional by checking other cloud provider implementations:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other implementations of default_function_name ast-grep --pattern 'def default_function_name($_, $resources: Optional[Resources] = None) -> str { $$$ }'Length of output: 109
I'll gather further evidence to see if any other cloud provider implementation uses a different approach for generating the default function name. Let's search for occurrences of the
default_function_namedefinition in the repository using a plain-text search.
🏁 Script executed:
#!/bin/bash rg "def default_function_name" -n .Length of output: 473
I'll compare the Azure implementation with another cloud provider implementation (AWS) to determine whether omitting the
resourcesparameter is intentional. Let's review the AWS implementation for context.
🏁 Script executed:
#!/bin/bash rg -C 10 "def default_function_name" sebs/aws/aws.pyLength of output: 887
I'll gather additional context from another cloud provider—GCP—to see how they handle the
resourcesparameter in naming.
🏁 Script executed:
#!/bin/bash rg -C 10 "def default_function_name" sebs/gcp/gcp.pyLength of output: 831
Resource ID Usage in Azure Default Function Naming is Intentional
In Azure’s implementation, the method always obtains the resource ID fromself.config.resourcesrather than from the providedresourcesparameter. This is in contrast with the AWS and GCP implementations—which are static methods and conditionally use the passedresourcesif available—but appears to be an intentional design decision for Azure to reference its instance configuration directly.
|
hi @mcopik I have made the required changes |
Summary by CodeRabbit
Release Notes
Configuration
Functionality
Versioning