Conversation
Correctly get regression cache path, select architecture, and pass resource prefix
📝 WalkthroughWalkthroughAdds a CircleCI regression pipeline and job; extends the CLI to choose architectures and normalize cache paths; parameterizes regression with a global RESOURCE_PREFIX and architecture-aware filtering; removes install_requires handling from one Python benchmark wrapper setup. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.circleci/config.yml:
- Around line 31-34: The CircleCI job currently caches "cache/" but the CLI
(sebs.cli) rewrites the default regression cache to "regression-cache" when
--cache is omitted, so the workflow never preserves the actual regression
metadata; update the CircleCI cache paths to include "regression-cache/" (or
change the CLI invocation to explicitly pass "--cache regression-cache" where
the job runs the CLI) and ensure both the save_cache entry (sebs-cache-{{
.Branch }} paths) and the job's CLI invocation are consistent (use
regression-cache) so the regression metadata is persisted/restored correctly.
In `@sebs/cli.py`:
- Around line 478-483: The code currently reads
config["experiments"]["architecture"] into architecture which raises KeyError
when experiments or architecture is missing; change the lookup in the block
after parse_common_params to guard the access by using config.get("experiments",
{}).get("architecture") and fall back to "x64" (or, if sebs_client is available
and selected_architecture is false, call sebs_client.get_experiment_config(...)
to derive the architecture) before passing architecture into regression_suite();
ensure you reference parse_common_params, the local variable architecture,
selected_architecture, sebs_client.get_experiment_config, and regression_suite
when making the change.
In `@sebs/regression.py`:
- Around line 1184-1186: The function initialize currently overwrites the
module-level RESOURCE_PREFIX when callers pass resource_prefix=None; change the
behavior so None means “don't clobber the existing default.” In initialize (and
the adjacent assignments around the
resource_prefix/benchmark_name/selected_architecture handling at the 1208-1210 /
1209-1210 area), only assign to the module-level variables if the incoming
parameter is not None (e.g., if resource_prefix is not None: RESOURCE_PREFIX =
resource_prefix), leaving the existing "regr" fallback intact when callers pass
None; apply the same guard for any similar parameters mentioned (benchmark_name,
selected_architecture) so None does not overwrite defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef7a4bac-cacf-4c1d-a59a-4f8ed21ada03
📒 Files selected for processing (3)
.circleci/config.ymlsebs/cli.pysebs/regression.py
| (config, output_dir, logging_filename, sebs_client, _) = parse_common_params( | ||
| initialize_deployment=False, | ||
| storage_configuration=storage_configuration, | ||
| **kwargs, | ||
| ) | ||
| architecture = config["experiments"]["architecture"] if selected_architecture else None |
There was a problem hiding this comment.
Guard the architecture lookup before filtering.
parse_common_params() is still holding the raw JSON dict, so the default from sebs/experiments/config.py:39-50 is not guaranteed to exist here. On Line 483, --selected-architecture will raise KeyError whenever the config file omits experiments.architecture. Fall back to "x64" or derive the value from sebs_client.get_experiment_config(...) before calling regression_suite().
Suggested fix
- architecture = config["experiments"]["architecture"] if selected_architecture else None
+ architecture = None
+ if selected_architecture:
+ architecture = config.get("experiments", {}).get("architecture") or "x64"🧰 Tools
🪛 Ruff (0.15.6)
[warning] 478-478: Unpacked variable output_dir is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 478-478: Unpacked variable logging_filename is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sebs/cli.py` around lines 478 - 483, The code currently reads
config["experiments"]["architecture"] into architecture which raises KeyError
when experiments or architecture is missing; change the lookup in the block
after parse_common_params to guard the access by using config.get("experiments",
{}).get("architecture") and fall back to "x64" (or, if sebs_client is available
and selected_architecture is false, call sebs_client.get_experiment_config(...)
to derive the architecture) before passing architecture into regression_suite();
ensure you reference parse_common_params, the local variable architecture,
selected_architecture, sebs_client.get_experiment_config, and regression_suite
when making the change.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.circleci/config.yml (1)
31-34:⚠️ Potential issue | 🟠 MajorCache path mismatch:
cache/vsregression-cache/.The CLI rewrites the default cache path from
cachetoregression-cachewhen--cacheis not specified (seesebs/cli.py:475-476). Sincerun-regression-testsdoesn't pass--cache, the regression metadata is written toregression-cache/, but this command only savescache/.Suggested fix
- save_cache: key: sebs-cache-{{ .Branch }} paths: - - cache/ + - regression-cache/,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 31 - 34, The CircleCI job "run-regression-tests" saves the wrong cache path: the save_cache step currently lists "cache/" but the CLI (sebs/cli.py default when --cache is omitted) writes regression metadata to "regression-cache/"; update the save_cache paths entry in the "run-regression-tests" job to "regression-cache/" (or include both "cache/" and "regression-cache/" if you want backward compatibility) so the saved cache matches where run-regression-tests actually writes data.
🧹 Nitpick comments (1)
.circleci/config.yml (1)
47-62: AWS credential setup is missing.The
setup-cloud-credentialscommand only handles GCP but the job invokes it withplatform: aws. While AWS credentials may be configured as CircleCI environment variables, consider adding explicit AWS credential handling for consistency and clarity.Suggested addition for AWS
setup-cloud-credentials: description: "Configure cloud authentication" parameters: platform: type: enum enum: [aws, azure, gcp] steps: + - when: + condition: + equal: [aws, << parameters.platform >>] + steps: + - run: + name: Verify AWS Credentials + command: | + if [ -z "$AWS_ACCESS_KEY_ID" ] || [ -z "$AWS_SECRET_ACCESS_KEY" ]; then + echo "AWS credentials not configured" + exit 1 + fi - when: condition: equal: [gcp, << parameters.platform >>]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 47 - 62, The setup-cloud-credentials job only implements GCP handling; add an AWS branch under the setup-cloud-credentials steps that triggers when parameters.platform equals aws and exports/configures AWS credentials (e.g., write AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and optionally AWS_SESSION_TOKEN and AWS_REGION into $BASH_ENV or create ~/.aws/credentials) so jobs invoked with parameters.platform: aws get explicit credentials; modify the when block for setup-cloud-credentials (parameters.platform) to include a new when.equal case for "aws" and run a step to export the AWS env vars.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.circleci/config.yml:
- Around line 36-45: The install-sebs step currently declares a platform
parameter but always runs "pip install .", so platform-specific deps aren't
installed; update the install-sebs step (the job named install-sebs) to use the
platform parameter to invoke the repository's installer (e.g., run python3
install.py with the corresponding flag: for platform == "aws" pass --aws, for
"azure" pass --azure, for "gcp" pass --gcp) and include the existing --no-local
flag as appropriate; ensure the new command replaces the plain "pip install ."
invocation and still handles the case of running all platforms if needed (mirror
how linting uses python3 install.py --aws --azure --gcp --no-local).
- Around line 115-117: The artifact storage step uses store_artifacts with path
set to "cache" but should match the CLI's output directory "regression-cache";
update the store_artifacts path value from "cache" to "regression-cache" so the
artifact destination aligns with the CLI behavior and earlier cache fixes (look
for the store_artifacts block and its path key in the CircleCI config).
---
Duplicate comments:
In @.circleci/config.yml:
- Around line 31-34: The CircleCI job "run-regression-tests" saves the wrong
cache path: the save_cache step currently lists "cache/" but the CLI
(sebs/cli.py default when --cache is omitted) writes regression metadata to
"regression-cache/"; update the save_cache paths entry in the
"run-regression-tests" job to "regression-cache/" (or include both "cache/" and
"regression-cache/" if you want backward compatibility) so the saved cache
matches where run-regression-tests actually writes data.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 47-62: The setup-cloud-credentials job only implements GCP
handling; add an AWS branch under the setup-cloud-credentials steps that
triggers when parameters.platform equals aws and exports/configures AWS
credentials (e.g., write AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and optionally
AWS_SESSION_TOKEN and AWS_REGION into $BASH_ENV or create ~/.aws/credentials) so
jobs invoked with parameters.platform: aws get explicit credentials; modify the
when block for setup-cloud-credentials (parameters.platform) to include a new
when.equal case for "aws" and run a step to export the AWS env vars.
| - store_artifacts: | ||
| path: cache | ||
| destination: cache-snapshot/ |
There was a problem hiding this comment.
Artifact path should be regression-cache to match CLI behavior.
Consistent with the earlier cache path issue, the artifact storage also saves cache but the CLI writes to regression-cache/.
Suggested fix
- store_artifacts:
- path: cache
+ path: regression-cache
destination: cache-snapshot/📝 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.
| - store_artifacts: | |
| path: cache | |
| destination: cache-snapshot/ | |
| - store_artifacts: | |
| path: regression-cache | |
| destination: cache-snapshot/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.circleci/config.yml around lines 115 - 117, The artifact storage step uses
store_artifacts with path set to "cache" but should match the CLI's output
directory "regression-cache"; update the store_artifacts path value from "cache"
to "regression-cache" so the artifact destination aligns with the CLI behavior
and earlier cache fixes (look for the store_artifacts block and its path key in
the CircleCI config).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.circleci/config.yml (2)
31-34:⚠️ Potential issue | 🟠 MajorCache and artifact the directory the regression command actually uses.
sebs/cli.pyrewrites the default regression cache fromcachetoregression-cachewhen--cacheis omitted, which is exactly how this job invokes the CLI. Savingcache/here and artifactingcachelater means the workflow never persists the regression metadata it just produced.Proposed fix
- save_cache: key: sebs-cache-{{ .Branch }} paths: - - cache/ + - regression-cache/ ... sebs benchmark regression test \ --config configs/example.json \ + --cache regression-cache \ --deployment << parameters.platform >> \ --language << parameters.language >> \ --language-version << parameters.version >> \ --architecture x64 --selected-architecture \ --resource-prefix sebs-ci ... - store_artifacts: - path: cache + path: regression-cache destination: cache-snapshot/Also applies to: 79-86, 118-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 31 - 34, The CI is saving and artifacting the wrong directory ("cache/") while the CLI (sebs/cli.py) uses "regression-cache" by default; update the save_cache keys and artifact paths in the CircleCI job(s) to use "regression-cache/" instead of "cache/" (and adjust any matching artifact steps) so the regression metadata produced by the regression command is persisted; apply the same change for the other occurrences referenced (the other save_cache/artifact blocks).
36-45:⚠️ Potential issue | 🟠 Major
install-sebsstill ignores itsplatformparameter.This reusable command is described as installing platform-specific dependencies, but Line 45 always runs
pip install .. If the cloud SDKs still come from the repo installer pattern already used bylintingon Lines 135-138, the regression job can fail before it even reachessebs benchmark regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 36 - 45, The reusable command "install-sebs" ignores its "platform" parameter; update its steps so the install command uses the platform value to install platform-specific SDKs (e.g., conditionalize the pip invocation using << parameters.platform >> to choose extras or run additional pip installs). Concretely, replace the single fixed "pip install ." run in the "install-sebs" command with a run step whose shell command branches on << parameters.platform >> (or maps it to pip install .[aws|azure|gcp] or pip install cloud-sdk-aws / cloud-sdk-azure / google-cloud-sdk as appropriate) so each platform installs its required dependencies before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sebs/cli.py`:
- Around line 485-490: The call to regression_suite is passing
kwargs["resource_prefix"] positionally which, when the CLI flag
--resource-prefix is omitted (making kwargs["resource_prefix"] None), overrides
the function default; update the call to either pass resource_prefix as a
keyword (e.g. resource_prefix=kwargs["resource_prefix"]) or substitute the
default when None (e.g. resource_prefix=kwargs["resource_prefix"] or "regr") so
the regression_suite function default is not unintentionally shadowed; locate
the call to regression_suite in sebs/cli.py and change the argument for
resource_prefix accordingly.
---
Duplicate comments:
In @.circleci/config.yml:
- Around line 31-34: The CI is saving and artifacting the wrong directory
("cache/") while the CLI (sebs/cli.py) uses "regression-cache" by default;
update the save_cache keys and artifact paths in the CircleCI job(s) to use
"regression-cache/" instead of "cache/" (and adjust any matching artifact steps)
so the regression metadata produced by the regression command is persisted;
apply the same change for the other occurrences referenced (the other
save_cache/artifact blocks).
- Around line 36-45: The reusable command "install-sebs" ignores its "platform"
parameter; update its steps so the install command uses the platform value to
install platform-specific SDKs (e.g., conditionalize the pip invocation using <<
parameters.platform >> to choose extras or run additional pip installs).
Concretely, replace the single fixed "pip install ." run in the "install-sebs"
command with a run step whose shell command branches on << parameters.platform
>> (or maps it to pip install .[aws|azure|gcp] or pip install cloud-sdk-aws /
cloud-sdk-azure / google-cloud-sdk as appropriate) so each platform installs its
required dependencies before proceeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c1d5516-b1b7-4dd5-a084-be56e020cb3b
📒 Files selected for processing (3)
.circleci/config.ymlbenchmarks/wrappers/aws/python/setup.pysebs/cli.py
💤 Files with no reviewable changes (1)
- benchmarks/wrappers/aws/python/setup.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sebs/regression.py (2)
1207-1217: Consider thread safety of global state mutation.The
RESOURCE_PREFIXandcloud_configglobals are modified at the start ofregression_suite, but tests run concurrently viaConcurrentStreamTestSuite. Ifregression_suitewere called multiple times concurrently (not currently the case based on CLI flow), this could cause race conditions. The current single-entry-point usage appears safe, but documenting this constraint would be beneficial.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sebs/regression.py` around lines 1207 - 1217, The regression_suite function mutates globals RESOURCE_PREFIX and cloud_config while tests run concurrently under ConcurrentStreamTestSuite, creating a potential race; change this by avoiding shared global mutation—either pass resource_prefix and deployment_config into tests via test case constructors or a context object (e.g., inject into setUp/setUpClass), or guard mutations with a global threading.Lock around assignments and document that regression_suite is not re-entrant; update references to RESOURCE_PREFIX and cloud_config inside test classes to read from the injected context or from a thread-local (threading.local) store so concurrent runs do not conflict.
1121-1150: Docstring is missing the new parameters.The docstring for
filter_out_benchmarksdocumentsarchitecturebut is missing documentation for thedeployment_typeandselected_architectureparameters that were added to the function signature.Suggested docstring update
Args: benchmark: The benchmark name to check deployment_name: Cloud provider name (aws, azure, gcp, openwhisk) language: Runtime language (python, nodejs) language_version: Language version (e.g., "3.9", "3.10") architecture: CPU architecture (x64, arm64) + deployment_type: Deployment type (package, container) + selected_architecture: If set, only include benchmarks matching this architecture Returns: bool: True if the benchmark should be included, False to filter it out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sebs/regression.py` around lines 1121 - 1150, The docstring for filter_out_benchmarks is missing the new parameters; update the Args section of the filter_out_benchmarks docstring to include deployment_type (str) describing the deployment category (e.g., "managed", "self-hosted" or provider-specific meaning used by your code) and selected_architecture (str | None) describing the optional user-requested architecture filter, and ensure their descriptions match the existing style for benchmark, deployment_name, language, language_version, and architecture; keep the Returns section as-is but adjust wording if needed to clarify True means "include" vs "filter out" to match the function behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sebs/regression.py`:
- Around line 1207-1217: The regression_suite function mutates globals
RESOURCE_PREFIX and cloud_config while tests run concurrently under
ConcurrentStreamTestSuite, creating a potential race; change this by avoiding
shared global mutation—either pass resource_prefix and deployment_config into
tests via test case constructors or a context object (e.g., inject into
setUp/setUpClass), or guard mutations with a global threading.Lock around
assignments and document that regression_suite is not re-entrant; update
references to RESOURCE_PREFIX and cloud_config inside test classes to read from
the injected context or from a thread-local (threading.local) store so
concurrent runs do not conflict.
- Around line 1121-1150: The docstring for filter_out_benchmarks is missing the
new parameters; update the Args section of the filter_out_benchmarks docstring
to include deployment_type (str) describing the deployment category (e.g.,
"managed", "self-hosted" or provider-specific meaning used by your code) and
selected_architecture (str | None) describing the optional user-requested
architecture filter, and ensure their descriptions match the existing style for
benchmark, deployment_name, language, language_version, and architecture; keep
the Returns section as-is but adjust wording if needed to clarify True means
"include" vs "filter out" to match the function behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79bc7614-ad47-4cc4-831a-ad44958a81a4
📒 Files selected for processing (2)
.circleci/config.ymlsebs/regression.py
1 participant
Summary by CodeRabbit
New Features
Chores