Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds platform-specific system-variant validation, updates AWS Lambda role and log handling, and adjusts benchmark metadata, dependencies, documentation, and regression tests for runtime compatibility. ChangesBenchmark and AWS updates
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains from the reviewed changes. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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)
tests/test_aws_matrix_fixes.py (3)
98-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest both accepted root-parent conventions.
Lines 101-105 test only the root-parent value
0. Lines 106-110 test an invalid value. Neither assertion covers the second validigraphroot-parent representation.Add a successful validation assertion for the alternate accepted root-parent value. Keep the checksum-mismatch assertion for an invalid value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_aws_matrix_fixes.py` around lines 98 - 110, Update test_igraph_root_parent_conventions_validate_equally to assert successful validation for the alternate accepted igraph root-parent value before changing result[2][0] to an invalid value. Preserve the existing checksum-mismatch assertion for the invalid root-parent case.
90-96: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest rejection during benchmark initialization.
This test checks
BenchmarkConfig.supports_system_variant. It does not execute the benchmark initialization path that must reject AWSpackagedeployments.Add a test that selects AWS
packagethrough the public initialization path and asserts the expected validation error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_aws_matrix_fixes.py` around lines 90 - 96, Extend the tests around test_411_is_container_only_on_aws to invoke the public benchmark initialization path with AWS and package selected, rather than only calling BenchmarkConfig.supports_system_variant. Assert that initialization raises the expected validation error, while preserving the existing configuration capability checks if still useful.
42-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the absent-role creation path.
Line 46 returns an existing role. This test does not execute
create_roleafterNoSuchEntityException. A failure in default-role creation can pass this regression test.Add a case where
get_roleraises the client exception. Assertcreate_roleand the DynamoDB policy attachment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_aws_matrix_fixes.py` around lines 42 - 72, Extend test_new_default_lambda_role_receives_dynamodb_access to cover the missing-role path by configuring iam_client.get_role to raise the AWS NoSuchEntityException, then assert resources.lambda_role invokes create_role and attaches the sebs-dynamodb-access policy to the created default role. Preserve the existing assertions for policy contents and idempotent behavior where applicable.sebs/benchmark.py (1)
638-649: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the constructor validation path.
The current regression test calls
BenchmarkConfig.supports_system_variantdirectly. It does not exercise the newBenchmark.__init__guard or theSystemVariant.valueconversion at Line 639. Add a focused test that rejects AWS/package and accepts AWS/container.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sebs/benchmark.py` around lines 638 - 649, Extend the regression tests to instantiate Benchmark through its __init__ validation path rather than only calling BenchmarkConfig.supports_system_variant. Add focused cases verifying AWS/package is rejected and AWS/container is accepted, exercising the SystemVariant.value conversion and the guard around supports_system_variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@benchmarks/500.scientific/504.dna-visualisation/python/requirements.txt.3.10`:
- Line 7: Remove the unnecessary pillow==10.3.0 entry from the Python 3.10
requirements file; the Lambda handler’s imports do not require Pillow, so leave
only the dependencies needed by the handler.
---
Nitpick comments:
In `@sebs/benchmark.py`:
- Around line 638-649: Extend the regression tests to instantiate Benchmark
through its __init__ validation path rather than only calling
BenchmarkConfig.supports_system_variant. Add focused cases verifying AWS/package
is rejected and AWS/container is accepted, exercising the SystemVariant.value
conversion and the guard around supports_system_variant.
In `@tests/test_aws_matrix_fixes.py`:
- Around line 98-110: Update
test_igraph_root_parent_conventions_validate_equally to assert successful
validation for the alternate accepted igraph root-parent value before changing
result[2][0] to an invalid value. Preserve the existing checksum-mismatch
assertion for the invalid root-parent case.
- Around line 90-96: Extend the tests around test_411_is_container_only_on_aws
to invoke the public benchmark initialization path with AWS and package
selected, rather than only calling BenchmarkConfig.supports_system_variant.
Assert that initialization raises the expected validation error, while
preserving the existing configuration capability checks if still useful.
- Around line 42-72: Extend
test_new_default_lambda_role_receives_dynamodb_access to cover the missing-role
path by configuring iam_client.get_role to raise the AWS NoSuchEntityException,
then assert resources.lambda_role invokes create_role and attaches the
sebs-dynamodb-access policy to the created default role. Preserve the existing
assertions for policy contents and idempotent behavior where applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86cadfeb-d5c6-40f3-92eb-14c72d32916e
📒 Files selected for processing (13)
benchmarks/100.webapps/130.crud-api/README.mdbenchmarks/400.inference/411.image-recognition/README.mdbenchmarks/400.inference/411.image-recognition/config.jsonbenchmarks/500.scientific/503.graph-bfs/README.mdbenchmarks/500.scientific/503.graph-bfs/input.pybenchmarks/500.scientific/504.dna-visualisation/README.mdbenchmarks/500.scientific/504.dna-visualisation/python/requirements.txt.3.10docs/build.mddocs/platforms.mdsebs/aws/aws.pysebs/aws/config.pysebs/benchmark.pytests/test_aws_matrix_fixes.py
| ret.logging_handlers = handlers | ||
| ret.logging.info("No resources for AWS found, initialize!") | ||
|
|
||
| configured_lambda_role = config.get("lambda-role") or config.get("resources", {}).get( |
There was a problem hiding this comment.
Why do we need that? Shouldn't this be set in initialize?
There was a problem hiding this comment.
I added this because the role set in the config could be ignored when a cached role exists. It also picks up aws.lambda-role, which is used in the examples but isn't passed to initialize().
You're right about the placement. I'd move it into initialize() and make sure the user's role takes priority over the cached one.
I also noticed that the examples/docs and the code use different places for lambda-role. Should I make those consistent in this PR too?
The follow jsonc block shows the two fields I mean, and user setting one of them should be sufficient:
{
"deployment": {
"aws": {
// Used in the examples/docs
"lambda-role": "arn:aws:iam::123456789012:role/my-role",
"resources": {
// Read by AWSResources.initialize()
"lambda-role": "arn:aws:iam::123456789012:role/my-role"
}
}
}
}| "checksum mismatch", | ||
| validate_output(None, {"size": 10, "seed": 42}, {"result": result}, "python"), | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think we can skip that test. It doesn't really test the actual deployment, but hardcodes that 3.10 and 3.11 should have the requirements file - unsure if that is needed.
| self.assertEqual(iam_client.put_role_policy.call_count, 1) | ||
|
|
||
| def test_configured_or_cached_lambda_role_is_not_modified(self): | ||
| role_arn = "arn:aws:iam::123456789012:role/sebs-lambda-role" |
There was a problem hiding this comment.
Hmm that is for what purpose?
|
Hi @rabbull! Do you have a moment to address my comments? I would like to merge the PR :) |
…arks into fix/aws-failures
2 participants
I was running SeBS on AWS Lambda to collect invocation-level performance data across all public benchmark, language, runtime, and deployment combinations. The initial matrix exposed several independent failures in benchmark packaging, deployment, validation, permissions, and AWS log parsing.
120.uploadermetricsSTARTandREPORTrecords, which confused the AWS report parser.130.crud-api411.image-recognitionsystem_variantsrestrictions and declare this benchmark container-only on AWS.503.graph-bfsigraphversions represent the BFS root parent as either the root itself or-1, causing equivalent results to fail checksum validation.504.dna-visualisationThe AWS documentation now also distinguishes the IAM identity running SeBS from the Lambda execution role and lists a broad quick-start permission bundle covering Lambda, S3, IAM, CloudWatch Logs, DynamoDB, and ECR.
I tested it through all valid configuration conbinations and now all benchmarks deploy and run on AWS Lambda.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation