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
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: 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 changes please review the required changes |
|
Hi @ojninja16 I just tested the PR and I'm not sure it works as expected. This command It created the function as expected but it add |
|
Hi @mcopik hope you are doing good, my understanding was that the requirement of the issue was to append the resource_id to the function names, was the expectation different? Please let me know I will make changes according to that |
|
@ojninja16 Exactly, but I can't find the resource_id in the function name :-) It only has language, language version and architecture. |
|
@mcopik that's weird. I will check for all the platforms once and make the necessary changes to make it work |
|
@ojninja16 Maybe the condition for resources malfunctioned :) I just started a first invocation from a clean cache. On a related note, would it be possible to change the function name template to: |
|
@mcopik so did it work?.Yeah that should not be an issue , I will do that🙂 |
|
@ojninja16 Invocation worked but the function didn't have the resource id in its name :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sebs/aws/aws.py (1)
393-393: Fix missing space after comma in function parameter.There's a minor formatting issue - missing space after the comma between parameters.
- self,code_package: Benchmark, resources: Optional[Resources] = None + self, code_package: Benchmark, resources: Optional[Resources] = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sebs/aws/aws.py(1 hunks)sebs/gcp/gcp.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sebs/gcp/gcp.py
🔇 Additional comments (1)
sebs/aws/aws.py (1)
393-394:❓ Verification inconclusive
Verify resource_id implementation across all platforms.
Ensure that this change is consistently implemented across all platforms (GCP, Local, OpenWhisk, Azure, System) as mentioned in the PR objectives to maintain uniformity in resource identification.
🏁 Script executed:
#!/bin/bash # Check if the resource_id implementation is consistent across all platforms echo "Checking resource_id implementation across platforms..." echo "AWS implementation:" rg -A 15 "default_function_name" sebs/aws/aws.py echo "\nGCP implementation:" rg -A 15 "default_function_name" sebs/gcp/gcp.py echo "\nLocal implementation:" rg -A 15 "default_function_name" sebs/local/local.py echo "\nOpenWhisk implementation:" rg -A 15 "default_function_name" sebs/openwhisk/openwhisk.py echo "\nAzure implementation:" rg -A 15 "default_function_name" sebs/azure/azure.py echo "\nSystem implementation:" rg -A 15 "default_function_name" sebs/faas/system.pyLength of output: 4975
Verify Resource ID Implementation Consistency Across All Platforms
I've verified that the implementations for generating the function name use the
resources_iddifferently across platforms:
AWS, GCP, and Azure:
They obtain the resource identifier directly viaself.config.resources.resources_idin the format string.Local and OpenWhisk:
They conditionally use theresourcesparameter (i.e.resources.resources_id) when provided, falling back to a version that omits it if not.System:
The implementation remains abstract, which is expected.Please confirm that this variance is intentional per the PR objectives. If a uniform approach is required, the Local and OpenWhisk implementations may need to be adjusted for consistency.
|
@mcopik I have made the changes for the fix please test it if the resource Id is being added to the name and added the resource Id in the prefix instead of suffix . |
|
@ojninja16 I just tested this and it's almost perfect :D Thank you! Two last minor corrections:
As soon as this is done then we can merge :) |
|
@mcopik glad it worked finally 😅! Yeah sure I will make those changes and let you know |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sebs/gcp/gcp.py (1)
111-111: Consider adding a null check for resources.The code assumes that either
resourcesorself.config.resourceswill be non-None. While this might be handled elsewhere in the codebase, it's safer to add a null check to prevent potentialAttributeErrorexceptions.- resource_id = resources.resources_id if resources else self.config.resources.resources_id + resource_id = resources.resources_id if resources else (self.config.resources.resources_id if self.config.resources else "default")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
sebs/aws/aws.py(1 hunks)sebs/azure/azure.py(2 hunks)sebs/config.py(2 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/openwhisk/openwhisk.py
- sebs/azure/azure.py
- sebs/aws/aws.py
- sebs/config.py
- sebs/local/local.py
🔇 Additional comments (2)
sebs/gcp/gcp.py (2)
112-117: Function name format aligns with PR requirements.The function name now includes the resource_id as a prefix, which matches the requirement mentioned in the PR comments to "prefix the resource_id to the function name instead of being added as a suffix". This will help with resource management and cleanup processes.
106-118:❓ Verification inconclusive
Correct implementation of the resource_id prefix in function names.
The modification correctly changes the
default_function_namemethod to add theresource_idas a prefix to the function name, which aligns with the PR objective. The method now accepts an optionalresourcesparameter and falls back toself.config.resources.resources_idif not provided.However, let's verify that all calls to this method have been updated since it's no longer a static method:
🏁 Script executed:
#!/bin/bash # Description: Find all calls to GCP.default_function_name that need to be updated # Search for static method calls that need to be changed rg "GCP\.default_function_name" --type pyLength of output: 43
Attention: Confirm Call Site Updates for
default_function_nameThe updated
default_function_namemethod insebs/gcp/gcp.pycorrectly uses the instance (viaself) to derive theresource_id. Our search for static calls using the pattern"GCP.default_function_name"produced no results, which suggests that the older static invocation pattern has been removed. However, since the search output is limited, please manually verify that all instances calling this method in the codebase are updated to use the instance form.
- Confirm that no remaining static calls (e.g.,
GCP.default_function_name(...)) exist.- Double-check any call sites that invoke
default_function_name(likely as an instance method) to ensure they work as expected.
|
Hi @mcopik, I’ve made the changes as you suggested! Looking forward to seeing the PR merged 😄 |
|
Thank you @ojninja16, great work!! |
2 participants
Summary by CodeRabbit
Release Notes
Configuration
Functionality
Versioning