Repository navigation
[Caliper] Work on the artifact export - #110
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test fournos llm_d smoke |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (45)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR restructures the Caliper postprocess pipeline into named stages (artifacts_to_kpis, kpis_to_csv, artifacts_to_ai_data, s3_import, analyse_kpis, s3_export), adds new S3 import/export and KPI-analysis modules, extends postprocess/final-status config, and adds vault discovery helpers used by CI resolvers, export, and replot flows, plus a typed FOURNOS job failure error. ChangesS3 pipeline and postprocess orchestration
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 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 |
🔴 Submission of
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/core/library/export.py (1)
561-568: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a distinct overall marker for
StepStatus.ONGOINGStepStatus.ONGOINGis already rendered as🔄for the current step, but the overall notification header collapses it to🟢, making an in-progress run look identical to a fully successful one. That also conflicts with the stated priority order (failure > ongoing > unknown > success). If the header is meant to reflect an active run, return a separate ongoing indicator here.🤖 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 `@projects/core/library/export.py` around lines 561 - 568, The overall status marker logic in the status selection helper currently maps StepStatus.ONGOING to the same green indicator as success, which hides active runs and conflicts with the intended priority order. Update the branch in the status helper that checks step_statuses so StepStatus.ONGOING returns a distinct ongoing marker instead of reusing the success marker, while keeping the existing failure and unknown handling unchanged.
🧹 Nitpick comments (5)
projects/core/ci_entrypoint/fournos_resolve.py (1)
196-204: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
locals()introspection with explicit initialization.
job_name/namespaceare only bound whenfetch_fournos_job()succeeds; on the caughtValueErrorthey remain unbound, so the code relies on"job_name" not in locals()to avoid aNameErrorat Line 202. This works only because theis_ttyguard and the earlierdry_runreturn (Line 192) happen to cover every unbound path — a fragile coupling. Initialize the names before thetryand test them directly so the safety no longer depends on reflection and branch ordering.♻️ Suggested refactor
# Check if we're in an interactive TTY environment is_tty = is_interactive_tty() + job_name = None + namespace = None + # Fetch the FournosJob object try: job_name, namespace, fjob_obj = fetch_fournos_job()- # Check if we have valid job info for cluster update - if is_tty and ("job_name" not in locals() or "namespace" not in locals()): + # Check if we have valid job info for cluster update + if is_tty and (job_name is None or namespace is None): logger.info("TTY MODE: Not applying changes to cluster (no FournosJob available)") return 0🤖 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 `@projects/core/ci_entrypoint/fournos_resolve.py` around lines 196 - 204, The TTY guard in fournOS_resolve should not depend on locals() to detect whether job_name and namespace were assigned. Initialize job_name and namespace before the try around fetch_fournos_job(), set them explicitly on success, and then check them directly before calling update_fournos_job(). Keep the existing control flow in resolve logic, but remove the reflection-based locals() introspection so the unbound-variable safety is explicit and tied to the symbols job_name, namespace, fetch_fournos_job, and update_fournos_job.projects/core/library/export.py (1)
745-746: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSTUB markers indicate incomplete export-vault resolution.
caliper_export_list_vaultsis annotated as a STUB (here and at Line 796). Since it feeds vault initialization for replot and the fournos resolve entrypoint, incomplete coverage could silently omit a required vault and surface only later as a downstream credential/auth failure. Want me to open a tracking issue for completing the export-vault set (additional providers/artifact repos)?🤖 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 `@projects/core/library/export.py` around lines 745 - 746, The export vault resolution logic in caliper_export_list_vaults is still marked as a STUB and may omit required vaults for replot and the fournos resolve entrypoint. Replace the placeholder behavior with full provider/artifact-repo coverage so all vaults needed by export operations are returned, and remove the STUB markers once the implementation is complete.projects/caliper/orchestration/postprocess.py (1)
1014-1047: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ai_eval_diris recomputed here instead of reusing the value already produced by_run_ai_eval_export.
_run_ai_eval_export's success result already includes"ai_eval_dir": str(ai_eval_dir)(per the returned payload). Recomputingoutput_dir / self.config.kpi.ai_eval_export.output_dirat Line 1025 duplicates that logic and could drift if the two ever compute the path differently.- ai_eval_dir = output_dir / self.config.kpi.ai_eval_export.output_dir + ai_eval_dir = Path(self.steps["ai_eval_export"]["ai_eval_dir"])🤖 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 `@projects/caliper/orchestration/postprocess.py` around lines 1014 - 1047, `_run_s3_export_step` is recomputing `ai_eval_dir` instead of reusing the path already returned by `_run_ai_eval_export`. Update the `ai_eval_dir` assignment inside `_run_s3_export_step` to read the existing value from `self.steps["ai_eval_export"]` when that step succeeded, and pass that stored path through to `run_s3_export`; keep the fallback only if the prior step did not produce it. This keeps the S3 export logic aligned with the `ai_eval_export` payload and avoids duplicate path construction.projects/caliper/orchestration/s3_export.py (1)
112-120: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
list_buckets()requires broads3:ListAllMyBucketspermission just to validate credentials.A least-privilege bucket policy scoped only to the target bucket (e.g.,
s3:PutObjectonarn:aws:s3:::{bucket}/*) would fail this check even though uploads would otherwise succeed. Considers3_client.head_bucket(Bucket=bucket)(scoped to the target bucket) instead.🤖 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 `@projects/caliper/orchestration/s3_export.py` around lines 112 - 120, The credential validation in the S3 export flow is too broad because the client setup uses s3_client.list_buckets(), which requires permissions beyond a least-privilege target-bucket policy. Update the S3 validation logic in the export client setup to verify access against the specific bucket being used by switching to a bucket-scoped check such as s3_client.head_bucket(Bucket=...) while keeping the boto3.client('s3') creation and logger success path intact.projects/caliper/orchestration/postprocess_config.py (1)
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winField descriptions contradict the
enabledvalidator.
instanceanddirectoryare documented as"(optional)", but_required_fields_when_enabled(Lines 194-209) raises aValueErrorrequiring both to be non-empty wheneverenabled=True. This will mislead anyone configurings3_exportfrom the docstring alone.✏️ Proposed fix
instance: str | None = Field( default=None, - description="Instance identifier for S3 export organization (optional).", + description="Instance identifier for S3 export organization (required when enabled).", ) directory: str | None = Field( default=None, - description="Directory identifier for S3 export organization (optional).", + description="Directory identifier for S3 export organization (required when enabled).", )🤖 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 `@projects/caliper/orchestration/postprocess_config.py` around lines 161 - 168, The `S3ExportConfig` field descriptions for `instance` and `directory` conflict with the `_required_fields_when_enabled` validator in `postprocess_config.py`: they are labeled optional even though `enabled=True` requires both values. Update the `Field(..., description=...)` text for `instance` and `directory` to clearly state they are required when `enabled` is set, and keep the wording aligned with the validation logic in `_required_fields_when_enabled` so the docs match runtime behavior.
🤖 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 `@projects/caliper/orchestration/postprocess_config.py`:
- Around line 96-100: The `kpi.ai_eval_export.output_dir` field in
`AiEvalExportConfig` should not allow null because `AIEvalExportConfig` and its
path-building call sites treat it as a required path segment. Change the type
from `str | None` to `str`, keep the existing default/value semantics, and
ensure any references in the post-processing config path logic continue to use a
guaranteed string so `output_dir / self.config.kpi.ai_eval_export.output_dir`
cannot raise a `TypeError`.
In `@projects/caliper/orchestration/s3_export.py`:
- Around line 190-224: The S3 export path builder in the s3 export flow is
ignoring the configured prefix, so `s3_config.prefix` has no effect on where
files are uploaded. Update the `export_s3_prefix` construction in the S3 export
logic to incorporate `prefix` along with `instance`, `directory`, and
`export_id`, and make sure the resulting key format used by the upload path
reflects the user-provided prefix. Keep the existing `s3_config` and
`export_s3_prefix` handling consistent so the returned result and actual upload
destination match.
- Around line 3-11: The import block in s3_export.py is failing Ruff I001
because the imports are not grouped and ordered correctly. Reorder the imports
in the module’s top-level import section so stdlib imports stay together, the
third-party yaml import is grouped separately, and the from
datetime/pathlib/typing imports are placed consistently with the rest of the
sorted block. Keep the change limited to the import section and verify it with
Ruff.
- Around line 268-291: The upload key selection in s3_export.py currently
matches generic CSVs before ai_eval CSVs, so files under ai_eval_dir can lose
their ai_eval/ prefix and collide with top-level CSVs. Update the branch order
in the upload loop so the ai_eval_dir check in the s3_key selection runs before
the include_csv .csv case, using the existing ai_eval_dir, export_s3_prefix, and
upload_plan logic to preserve the nested path for ai_eval files.
- Around line 97-127: Avoid mutating AWS_SHARED_CREDENTIALS_FILE inside
create_s3_client; instead, create a dedicated botocore.session.Session, set its
credentials_file to the provided credentials_path, and build the S3 client
through boto3.Session(botocore_session=...). Keep the existing authentication
test and error handling in create_s3_client, but ensure the credentials path is
scoped to this function so other AWS calls in the process are not affected.
In `@projects/core/library/postprocess.py`:
- Around line 70-81: The configuration export block is calling
Config.save_config with an unsupported dest argument, so the config file is
never written and the error gets hidden by the broad exception handler. Update
the postprocess() logic to match the actual Config.save_config signature in
config.py by either removing the dest=config_path argument or changing
Config.save_config to accept a destination path, and keep the
logger.info/logger.warning handling around that same save_config call.
In `@projects/core/library/replot.py`:
- Around line 39-74: The blanket exception handling in the replot vault
initialization flow is hiding failures that should stop the operation early. In
the replot setup around the vault collection and vault.init path, keep the
logging but do not silently continue for required dependencies; explicitly
detect when replot_url is set and the MLflow-related vaults fail to initialize,
then raise or fail fast instead of swallowing the error. Update the logic in the
replot function so required vault initialization failures surface immediately,
while only nonessential vault issues can be downgraded if truly safe.
---
Outside diff comments:
In `@projects/core/library/export.py`:
- Around line 561-568: The overall status marker logic in the status selection
helper currently maps StepStatus.ONGOING to the same green indicator as success,
which hides active runs and conflicts with the intended priority order. Update
the branch in the status helper that checks step_statuses so StepStatus.ONGOING
returns a distinct ongoing marker instead of reusing the success marker, while
keeping the existing failure and unknown handling unchanged.
---
Nitpick comments:
In `@projects/caliper/orchestration/postprocess_config.py`:
- Around line 161-168: The `S3ExportConfig` field descriptions for `instance`
and `directory` conflict with the `_required_fields_when_enabled` validator in
`postprocess_config.py`: they are labeled optional even though `enabled=True`
requires both values. Update the `Field(..., description=...)` text for
`instance` and `directory` to clearly state they are required when `enabled` is
set, and keep the wording aligned with the validation logic in
`_required_fields_when_enabled` so the docs match runtime behavior.
In `@projects/caliper/orchestration/postprocess.py`:
- Around line 1014-1047: `_run_s3_export_step` is recomputing `ai_eval_dir`
instead of reusing the path already returned by `_run_ai_eval_export`. Update
the `ai_eval_dir` assignment inside `_run_s3_export_step` to read the existing
value from `self.steps["ai_eval_export"]` when that step succeeded, and pass
that stored path through to `run_s3_export`; keep the fallback only if the prior
step did not produce it. This keeps the S3 export logic aligned with the
`ai_eval_export` payload and avoids duplicate path construction.
In `@projects/caliper/orchestration/s3_export.py`:
- Around line 112-120: The credential validation in the S3 export flow is too
broad because the client setup uses s3_client.list_buckets(), which requires
permissions beyond a least-privilege target-bucket policy. Update the S3
validation logic in the export client setup to verify access against the
specific bucket being used by switching to a bucket-scoped check such as
s3_client.head_bucket(Bucket=...) while keeping the boto3.client('s3') creation
and logger success path intact.
In `@projects/core/ci_entrypoint/fournos_resolve.py`:
- Around line 196-204: The TTY guard in fournOS_resolve should not depend on
locals() to detect whether job_name and namespace were assigned. Initialize
job_name and namespace before the try around fetch_fournos_job(), set them
explicitly on success, and then check them directly before calling
update_fournos_job(). Keep the existing control flow in resolve logic, but
remove the reflection-based locals() introspection so the unbound-variable
safety is explicit and tied to the symbols job_name, namespace,
fetch_fournos_job, and update_fournos_job.
In `@projects/core/library/export.py`:
- Around line 745-746: The export vault resolution logic in
caliper_export_list_vaults is still marked as a STUB and may omit required
vaults for replot and the fournos resolve entrypoint. Replace the placeholder
behavior with full provider/artifact-repo coverage so all vaults needed by
export operations are returned, and remove the STUB markers once the
implementation is complete.
🪄 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
Run ID: 00f4354f-8f12-4b2b-8fbd-9b108e66d90b
📒 Files selected for processing (14)
projects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_config.pyprojects/caliper/orchestration/postprocess_outcome.pyprojects/caliper/orchestration/s3_export.pyprojects/core/ci_entrypoint/fournos_resolve.pyprojects/core/library/export.pyprojects/core/library/postprocess.pyprojects/core/library/replot.pyprojects/guidellm/postprocess/guidellm/plugin.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/config.yamlprojects/skeleton/orchestration/ci.pyprojects/skeleton/orchestration/config.yamlvaults/psap-forge-aws-s3-export.yaml
ba5a078 to
1e2d881
Compare
|
/test fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
1 similar comment
|
/test fournos llm_d smoke |
🔴 Submission of
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
projects/fournos_launcher/orchestration/submit.py (1)
411-417: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend error passing to the parallel-job notification too.
Good addition for the single-job path. Note the parallel-job path (line 379-381) still calls
send_github_notification(...)withouterror=, even though per-job exceptions are already caught at lines 350-356 — parallel failures won't get the new granular error detail in the notification.🤖 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 `@projects/fournos_launcher/orchestration/submit.py` around lines 411 - 417, The parallel-job notification path still omits the error detail when calling send_github_notification, so mirror the single-job handling there. In the parallel branch of submit.py, reuse the already-captured per-job exception from the try/except around the job execution and pass it through the send_github_notification(...) call as error=..., keeping the success logic and job_type handling unchanged.
🤖 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 `@projects/caliper/orchestration/s3_export.py`:
- Around line 283-287: The fallback path handling in the upload flow is
producing invalid local paths when a file is not under artifact_dir. Update the
logic in the path normalization and upload code around the relative_local_path
handling and the upload step so that files outside artifact_dir keep their
original source path for the actual upload instead of being re-prefixed later.
Use the existing relative_local_path logic and the upload path construction in
the same flow to ensure the stored path and the file used for upload stay
consistent.
- Around line 234-241: Dry-run exports are incorrectly blocked by AWS credential
lookup in s3_export.py, so move the get_aws_credentials check out of the shared
setup path and into the actual upload flow only. Update the logic around the
export entry point and the upload code path so dry_run can still generate and
write the upload plan without requiring vault credentials, while the real upload
path continues to fail if credentials are missing. Reference the existing
s3_export helper flow and the credential handling block to keep the behavior
split cleanly.
- Line 215: The export_id generation in s3_export should be made
collision-resistant because datetime.now().strftime("%y-%m-%d_%H%M%S") can
repeat for exports started in the same second. Update the export_id logic in the
export flow to include higher-resolution time and/or a unique suffix so
concurrent exports for the same instance and directory cannot overwrite each
other. Keep the change localized to the export_id assignment path used by the S3
export operation.
---
Nitpick comments:
In `@projects/fournos_launcher/orchestration/submit.py`:
- Around line 411-417: The parallel-job notification path still omits the error
detail when calling send_github_notification, so mirror the single-job handling
there. In the parallel branch of submit.py, reuse the already-captured per-job
exception from the try/except around the job execution and pass it through the
send_github_notification(...) call as error=..., keeping the success logic and
job_type handling unchanged.
🪄 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
Run ID: a48e6a6a-d9fc-4552-8d18-ec7f88687390
📒 Files selected for processing (16)
projects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_config.pyprojects/caliper/orchestration/postprocess_outcome.pyprojects/caliper/orchestration/s3_export.pyprojects/core/ci_entrypoint/fournos_resolve.pyprojects/core/library/export.pyprojects/core/library/postprocess.pyprojects/core/library/replot.pyprojects/fournos_launcher/orchestration/submit.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/guidellm/postprocess/guidellm/plugin.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/config.yamlprojects/skeleton/orchestration/ci.pyprojects/skeleton/orchestration/config.yamlvaults/psap-forge-aws-s3-export.yaml
✅ Files skipped from review due to trivial changes (1)
- projects/guidellm/postprocess/guidellm/plugin.py
🚧 Files skipped from review as they are similar to previous changes (12)
- vaults/psap-forge-aws-s3-export.yaml
- projects/skeleton/orchestration/config.yaml
- projects/core/library/postprocess.py
- projects/core/library/replot.py
- projects/skeleton/orchestration/ci.py
- projects/caliper/orchestration/postprocess_outcome.py
- projects/llm_d/orchestration/config.yaml
- projects/caliper/orchestration/postprocess_config.py
- projects/core/ci_entrypoint/fournos_resolve.py
- projects/llm_d/orchestration/ci.py
- projects/caliper/orchestration/postprocess.py
- projects/core/library/export.py
|
/test fournos llm_d smoke |
2 similar comments
|
/test fournos llm_d smoke |
|
/test fournos llm_d smoke |
🟢 Execution of
|
1 similar comment
🟢 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/core/library/export.py (1)
560-568: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a distinct indicator for ongoing runs
_get_overall_status_from_steps()returns🟢for bothStepStatus.ONGOINGand completed success, so dashboards and notifications can't distinguish "still running" from "passed." Use a separate in-progress emoji here, such as🟡or🔄.🤖 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 `@projects/core/library/export.py` around lines 560 - 568, The _get_overall_status_from_steps() logic in export.py currently uses the same green indicator for StepStatus.ONGOING and completed success, so update the StepStatus.ONGOING branch to return a distinct in-progress symbol (for example, a yellow or rotating emoji) while keeping the success branch green; preserve the existing failure and unknown handling and make sure the priority order in the status checks still works as intended.projects/core/library/vault.py (1)
211-238: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestore strict failure for unexpected vault files
validate_vault()now only warns and emits a notification for non-secretsync*extras, so strict validation can still pass with unapproved files undersecret_dir. That breaks the vault’s “no extra files” rule; fail validation in strict mode or make the allowlist explicit.🤖 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 `@projects/core/library/vault.py` around lines 211 - 238, `validate_vault()` currently only logs and creates a CI notification for unexpected files under `secret_dir`, which allows strict validation to pass with unapproved vault contents. Update the extra-file handling in `projects.core.library.vault.validate_vault` so that non-`secretsync*` files still trigger a hard failure when strict validation is enabled, while preserving the existing warning/notification behavior only for explicitly allowed non-strict cases. Use the existing `defined_files`, `vault_name`, and `secret_dir` checks to locate the logic, and keep the `ci_lib.add_notification_file` path only as a secondary notification after the strict decision is made.
♻️ Duplicate comments (1)
projects/caliper/orchestration/s3_export.py (1)
117-117: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse
credentials_filehereprojects/caliper/orchestration/s3_export.py:117—shared_credentials_fileis ignored by the session variable lookup, so this scoped credentials file can be skipped and boto3 may fall back to ambient credentials.🔧 Proposed fix
- botocore_session.set_config_variable("shared_credentials_file", str(credentials_path)) + botocore_session.set_config_variable("credentials_file", str(credentials_path))🤖 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 `@projects/caliper/orchestration/s3_export.py` at line 117, The session variable is using the wrong credential key in s3_export’s setup, so the scoped credentials file may be ignored. Update the botocore session configuration in the relevant setup path to use credentials_file instead of shared_credentials_file, keeping the change in the same credentials-loading logic so boto3 picks up the intended file.
🧹 Nitpick comments (1)
projects/core/library/vault.py (1)
228-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNotification files for the same extra vault file will accumulate across repeated validation runs
add_notification_filealways picks the next unused indexed slot ({index:03d}__{name}.txt) rather than checking whether an equivalent notification already exists. Sincevalidate_vaultcan run multiple times for the same vault (mandatory + optional passes, multiple pipeline stages, replot vs. postprocess, etc.), the same extra file will keep generating new notification entries instead of being deduplicated, cluttering the notifications surfaced to operators.♻️ Suggested fix: dedupe by checking for an existing notification before creating a new one
if filename not in defined_files: msg = f"Vault '{vault_name}' contains extra file '{filename}' at '{file_path}' not defined in specification" # Always warn for extra files, never fail validation logger.warning(msg) # Add CI notification for extra vault files from projects.core.library import ci as ci_lib - notification_msg = f"Extra vault file detected: {vault_name}/{filename}\n\nThis file exists in the vault but is not defined in the vault specification. This is not an error but should be reviewed for security purposes." - ci_lib.add_notification_file( - name=f"vault_extra_file_{vault_name}_{filename}", - message=notification_msg, - ) + notification_name = f"vault_extra_file_{vault_name}_{filename}" + if not ci_lib.notification_exists(notification_name): + notification_msg = ( + f"Extra vault file detected: {vault_name}/{filename}\n\n" + "This file exists in the vault but is not defined in the vault " + "specification. This is not an error but should be reviewed for " + "security purposes." + ) + ci_lib.add_notification_file(name=notification_name, message=notification_msg)(requires adding a small
notification_exists-style helper inci.py, or reusing a deterministic filename with overwrite semantics instead of an auto-incrementing index)🤖 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 `@projects/core/library/vault.py` around lines 228 - 238, The extra-vault-file notification path in validate_vault is creating duplicate CI notification files on repeated runs because add_notification_file always allocates a new indexed filename. Update the vault validation flow around the ci_lib.add_notification_file call to first check whether the same notification already exists, or switch to a deterministic filename/overwrite approach; if needed, add a small notification_exists helper in ci.py and reuse it before writing the notification for the vault_name/filename pair.
🤖 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 `@projects/caliper/orchestration/s3_export.py`:
- Around line 382-396: The dry-run export path in s3_export.py still returns a
success response even when the file write fails, so update the try/except around
the dry-run save logic to treat write failures as a real failure instead of
continuing. In the dry-run branch that writes `dry_run_file` and builds the
response, make sure any exception from `open()` or `yaml.dump()` stops the
success response and returns an error/failure result from the same function,
using the existing `logger.error` context so callers do not receive a bogus
`dry_run_file` path.
- Around line 123-124: The preflight bucket listing in s3_export.py is too
strict because it requires account-wide list permissions before the upload can
begin. Remove the s3_client.list_buckets() credential test from the S3 setup
path and let the existing upload_file() call in the S3 export flow surface any
real permission or destination-path errors instead.
In `@projects/core/library/export.py`:
- Around line 693-711: The success log after calling vault.init() is misleading
because that function can return immediately when the vault manager is already
initialized, so the export-specific vaults are not actually revalidated. Update
the export setup in export.py to distinguish between “requested initialization”
and “actually initialized/validated,” using caliper_export_list_vaults() and
vault.init() as the key flow points, and only log success when the vaults were
truly initialized or checked; otherwise log that initialization was skipped
because the manager was already initialized.
---
Outside diff comments:
In `@projects/core/library/export.py`:
- Around line 560-568: The _get_overall_status_from_steps() logic in export.py
currently uses the same green indicator for StepStatus.ONGOING and completed
success, so update the StepStatus.ONGOING branch to return a distinct
in-progress symbol (for example, a yellow or rotating emoji) while keeping the
success branch green; preserve the existing failure and unknown handling and
make sure the priority order in the status checks still works as intended.
In `@projects/core/library/vault.py`:
- Around line 211-238: `validate_vault()` currently only logs and creates a CI
notification for unexpected files under `secret_dir`, which allows strict
validation to pass with unapproved vault contents. Update the extra-file
handling in `projects.core.library.vault.validate_vault` so that
non-`secretsync*` files still trigger a hard failure when strict validation is
enabled, while preserving the existing warning/notification behavior only for
explicitly allowed non-strict cases. Use the existing `defined_files`,
`vault_name`, and `secret_dir` checks to locate the logic, and keep the
`ci_lib.add_notification_file` path only as a secondary notification after the
strict decision is made.
---
Duplicate comments:
In `@projects/caliper/orchestration/s3_export.py`:
- Line 117: The session variable is using the wrong credential key in
s3_export’s setup, so the scoped credentials file may be ignored. Update the
botocore session configuration in the relevant setup path to use
credentials_file instead of shared_credentials_file, keeping the change in the
same credentials-loading logic so boto3 picks up the intended file.
---
Nitpick comments:
In `@projects/core/library/vault.py`:
- Around line 228-238: The extra-vault-file notification path in validate_vault
is creating duplicate CI notification files on repeated runs because
add_notification_file always allocates a new indexed filename. Update the vault
validation flow around the ci_lib.add_notification_file call to first check
whether the same notification already exists, or switch to a deterministic
filename/overwrite approach; if needed, add a small notification_exists helper
in ci.py and reuse it before writing the notification for the
vault_name/filename pair.
🪄 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
Run ID: 99e5333d-5ac6-45ef-b609-d195f44c4cf1
📒 Files selected for processing (17)
projects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_config.pyprojects/caliper/orchestration/postprocess_outcome.pyprojects/caliper/orchestration/s3_export.pyprojects/core/ci_entrypoint/fournos_resolve.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/library/config.pyprojects/core/library/export.pyprojects/core/library/replot.pyprojects/core/library/vault.pyprojects/fournos_launcher/orchestration/submit.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/config.yamlprojects/skeleton/orchestration/ci.pyprojects/skeleton/orchestration/config.yamlvaults/psap-forge-aws-s3-export.yaml
✅ Files skipped from review due to trivial changes (1)
- projects/core/ci_entrypoint/prepare_ci.py
🚧 Files skipped from review as they are similar to previous changes (12)
- projects/core/library/replot.py
- projects/skeleton/orchestration/ci.py
- projects/llm_d/orchestration/config.yaml
- projects/skeleton/orchestration/config.yaml
- projects/fournos_launcher/orchestration/submit.py
- projects/caliper/orchestration/postprocess_config.py
- projects/llm_d/orchestration/ci.py
- projects/caliper/orchestration/postprocess_outcome.py
- projects/core/ci_entrypoint/fournos_resolve.py
- vaults/psap-forge-aws-s3-export.yaml
- projects/caliper/orchestration/postprocess.py
- projects/fournos_launcher/toolbox/submit_and_wait/main.py
|
/test fournos llm_d smoke |
…e HTTPS warning if mlflow_insecure_tls
…f no workspace is parsed
🟠 Execution of
|
🟢 Submission of
|
|
merging this, I'll continue the work in another PR |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
https://github.com/coderabbitai full review |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes