Fix AWS Deployment Failures - #312
Conversation
📝 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
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
| # we have to fix version to prevent compilation from source | ||
| numpy==2.2.6 | ||
| contourpy==1.3.2 | ||
| pillow==10.3.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
curl -fsSL -X POST https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' \
--data '{"package":{"ecosystem":"PyPI","name":"Pillow"},"version":"10.3.0"}' |
jq -r '.vulns[]? | [.id, .summary] | `@tsv`'
curl -fsSL https://pypi.org/pypi/Pillow/12.2.0/json |
jq -r '.urls[].filename' |
grep -iE 'pillow-12\.2\.0-cp310-cp310-manylinux.*(x86_64|aarch64)'Repository: spcl/serverless-benchmarks
Length of output: 2985
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
FILE="benchmarks/500.scientific/504.dna-visualisation/python/requirements.txt.3.10"
echo "== requirements file =="
cat -n "$FILE" | sed -n '1,40p'
echo
echo "== references within dna-visualisation directory =="
python3 - <<'PY'
import pathlib, re
root = pathlib.Path("benchmarks/500.scientific/504.dna-visualisation")
for p in root.rglob("*"):
if p.is_file() and not any(part in {".git", "__pycache__", ".eggs"} for part in p.parts):
try:
text = p.read_text(errors="ignore")
except Exception:
continue
if re.search(r"\b(PIL|Pillow)\b|pillow\b|import Image|from PIL|Image\.", text, re.I):
print(p)
PYRepository: spcl/serverless-benchmarks
Length of output: 777
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== requirements/python files =="
for f in $(git ls-files benchmarks/500.scientific/504.dna-visualisation/python); do
echo "--- $f"
cat -n "$f" | sed -n '1,80p'
done
echo
echo "== python imports in benchmark source =="
python3 - <<'PY'
import pathlib, ast
root = pathlib.Path("benchmarks/500.scientific/504.dna-visualisation/python")
for p in sorted(root.glob("*.py")):
tree = ast.parse(p.read_text())
names = []
for node in ast.walk(tree):
if isinstance(node, ast.Import):
names.extend(a.name.split(".", 1)[0] for a in node.names)
elif isinstance(node, ast.ImportFrom):
names.append((node.module or "").split(".", 1)[0] or "")
print(p, sorted(n for n in names if n in {"PIL", "Pillow", "numpy", "matplotlib", "Pillow"} or n))
PYRepository: spcl/serverless-benchmarks
Length of output: 4684
Remove the unnecessary Pillow requirement.
pillow==10.3.0 is still pinned here, but the Lambda handler imports only datetime, io, json, os, and squiggle. Remove it to avoid loading a vulnerable dependency into the runtime unless a future dependency or use case requires it.
🤖 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 `@benchmarks/500.scientific/504.dna-visualisation/python/requirements.txt.3.10`
at 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.
1 participant
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