Performance Improvements - #10
ShashankFC wants to merge 2 commits into
Conversation
WalkthroughRefactors S3 utility functions to make object retrieval and decompression more explicit, and adds sorting to standardize test case iteration order before aggregation. No functional behavior changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tools/stats/upload_test_stats.py (1)
169-175: Deterministic sorting before aggregationSorting by
fileandclassnamebefore grouping makes the aggregation order stable without changing the aggregated values; this is a reasonable tradeoff in complexity for more predictable outputs. If you ever need ordering to be fully aligned withget_key, you could extend the sort key to include the remaining key fields, but it’s not required for correctness.tools/stats/upload_stats_lib.py (2)
75-83: Artifact run-attempt filter never actually skips artifactsThe
runattemptcheck logs that an artifact is being skipped but then continues to download it anyway; there is nocontinue/returnafter the mismatch is detected. That means artifacts from other run attempts will still be fetched and processed despite the warning.Consider explicitly skipping mismatched artifacts, for example:
- for atom in atoms: - if atom.startswith("runattempt"): - found_run_attempt = int(atom[len("runattempt") :]) - if workflow_run_attempt != found_run_attempt: - print( - f"Skipping {artifact_name} as it is an invalid run attempt. " - f"Expected {workflow_run_attempt}, found {found_run_attempt}." - ) + for atom in atoms: + if atom.startswith("runattempt"): + found_run_attempt = int(atom[len("runattempt") :]) + if workflow_run_attempt != found_run_attempt: + print( + f"Skipping {artifact_name} as it is an invalid run attempt. " + f"Expected {workflow_run_attempt}, found {found_run_attempt}." + ) + return artifact_name # or return early with a sentinel and filter at callerand adjust the caller if needed to avoid adding skipped artifacts to
paths.
166-171: Minor simplification in S3 object constructionThe new intermediate
s3_objimproves readability. You can simplify the call a bit by dropping unnecessary f-strings:- s3_obj = get_s3_resource().Object(f"{bucket_name}", f"{key}") + s3_obj = get_s3_resource().Object(bucket_name, key)Behavior is unchanged; this just tightens up the call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tools/stats/upload_stats_lib.py(3 hunks)tools/stats/upload_test_stats.py(1 hunks)
| for obj in objs: | ||
| object_name = Path(obj.key).name | ||
| # target an artifact for a specific job_id if provided, otherwise skip the download. | ||
| if job_id is not None and str(job_id) not in object_name: | ||
| continue | ||
| found_one = True | ||
| p = Path(Path(obj.key).name) | ||
| print(f"Downloading {p}") | ||
| s3_resource = get_s3_resource() | ||
| bucket_obj = s3_resource.Bucket(GHA_ARTIFACTS_BUCKET) | ||
| obj_data = bucket_obj.Object(obj.key).get() | ||
| with open(p, "wb") as f: | ||
| f.write(obj.get()["Body"].read()) | ||
| f.write(obj_data["Body"].read()) | ||
| paths.append(p) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reuse existing S3 bucket object instead of recreating it per object
Functionality is correct, but you’re re-acquiring the S3 resource and bucket inside the loop even though bucket is already created at Line 99. Reusing bucket is a bit clearer and avoids redundant calls:
@@ def download_s3_artifacts(
- p = Path(Path(obj.key).name)
- print(f"Downloading {p}")
- s3_resource = get_s3_resource()
- bucket_obj = s3_resource.Bucket(GHA_ARTIFACTS_BUCKET)
- obj_data = bucket_obj.Object(obj.key).get()
- with open(p, "wb") as f:
- f.write(obj_data["Body"].read())
+ p = Path(Path(obj.key).name)
+ print(f"Downloading {p}")
+ obj_body = bucket.Object(obj.key).get()["Body"].read()
+ with open(p, "wb") as f:
+ f.write(obj_body)This keeps the logic explicit while avoiding extra resource/bucket construction.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tools/stats/upload_stats_lib.py around lines 105 to 117, the loop recreates
the S3 resource and bucket for every obj even though a bucket variable was
created at line 99; replace the per-iteration get_s3_resource() and bucket_obj =
s3_resource.Bucket(...) calls by reusing the existing bucket (e.g., call
bucket.Object(obj.key).get()), remove the redundant resource/bucket creation,
and keep the rest of the download/write logic unchanged so each object is
fetched from the already-initialized bucket.
| decompressed = gzip.decompress(body).decode() | ||
| results = decompressed.split("\n") | ||
| return [json.loads(result) for result in results if result] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Decompression path matches upload; consider splitlines() for robustness
Adding gzip.decompress(body).decode() aligns read_from_s3 with upload_to_s3’s gzipped JSONL writes and should fix any mismatch there. For slightly better handling of different newline conventions, you could use splitlines() instead of splitting on "\n":
- decompressed = gzip.decompress(body).decode()
- results = decompressed.split("\n")
+ decompressed = gzip.decompress(body).decode()
+ results = decompressed.splitlines()The current implementation is still correct given how these payloads are produced.
📝 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.
| decompressed = gzip.decompress(body).decode() | |
| results = decompressed.split("\n") | |
| return [json.loads(result) for result in results if result] | |
| decompressed = gzip.decompress(body).decode() | |
| results = decompressed.splitlines() | |
| return [json.loads(result) for result in results if result] |
🤖 Prompt for AI Agents
In tools/stats/upload_stats_lib.py around lines 189 to 191, the code
decompresses gzipped JSONL and currently splits the text with .split("\n");
change that to use .splitlines() to robustly handle different newline
conventions (CR, LF, CRLF) and avoid empty-line issues, then continue to filter
out any empty strings and json.loads each non-empty line as before.
Entelligence AI Vulnerability ScannerStatus: No security vulnerabilities found Your code passed our comprehensive security analysis. Analyzed 2 files in total |
Review Summary❌ Rejected Comments (1)
🏷️ Draft Comments (6)
|
🔬 Multi-Approach Review SummaryThis PR was reviewed by 2 different approaches for comparison:
Total: 3 review comments Each comment is labeled with its source approach. This allows you to compare different AI review strategies. 🔒 Security Scan: Run once and shared across all approaches for efficiency. WalkthroughThis PR focuses on code quality improvements in the stats upload tooling. The changes include stylistic refactoring in Changes
Sequence DiagramThis diagram shows the interactions between components: sequenceDiagram
participant Client as Client Code
participant S3Service as S3 Resource Service
participant S3Bucket as S3 Bucket
participant S3Obj as S3 Object
participant FileSystem as Local File System
Note over Client,FileSystem: Artifact Download Flow (Change Block 1)
loop For each object in objs
Client->>Client: Check if job_id matches object_name
alt job_id matches or is None
Client->>S3Service: get_s3_resource()
S3Service-->>Client: s3_resource
Client->>S3Bucket: s3_resource.Bucket(GHA_ARTIFACTS_BUCKET)
S3Bucket-->>Client: bucket_obj
Client->>S3Obj: bucket_obj.Object(obj.key).get()
S3Obj-->>Client: obj_data
Client->>FileSystem: open(path, "wb")
FileSystem-->>Client: file handle
Client->>Client: f.write(obj_data["Body"].read())
Client->>FileSystem: close file
Client->>Client: paths.append(path)
end
end
Note over Client,S3Obj: Artifact Upload Flow (Change Block 2)
Client->>Client: Prepare gzipped body content
Client->>S3Service: get_s3_resource()
S3Service-->>Client: s3_resource
Client->>S3Obj: s3_resource.Object(bucket_name, key)
S3Obj-->>Client: s3_obj reference
Client->>S3Obj: s3_obj.put(Body, ContentEncoding, ContentType)
S3Obj-->>Client: Upload complete
Note over Client,S3Obj: Artifact Retrieval Flow (Change Block 3)
Client->>S3Obj: get()["Body"].read()
S3Obj-->>Client: compressed body
Client->>Client: gzip.decompress(body).decode()
Client->>Client: decompressed.split("\n")
Client->>Client: Parse JSON results
🔒 Security Analysis
Note for WindsurfPlease change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
Also you can trigger various commands with the bot by doing The current supported commands are
More commands to be added soon. |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| paths = [] | ||
| for obj in objs: | ||
| object_name = Path(obj.key).name | ||
| # target an artifact for a specific job_id if provided, otherwise skip the download. | ||
| if job_id is not None and str(job_id) not in object_name: | ||
| continue | ||
| found_one = True | ||
| p = Path(Path(obj.key).name) | ||
| print(f"Downloading {p}") | ||
| s3_resource = get_s3_resource() | ||
| bucket_obj = s3_resource.Bucket(GHA_ARTIFACTS_BUCKET) | ||
| obj_data = bucket_obj.Object(obj.key).get() | ||
| with open(p, "wb") as f: | ||
| f.write(obj.get()["Body"].read()) | ||
| f.write(obj_data["Body"].read()) | ||
| paths.append(p) | ||
|
|
||
| if not found_one: |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The s3_resource is initialized inside the loop for each object, which is unnecessary and inefficient. Move the initialization outside the loop to avoid redundant calls to get_s3_resource().
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| paths = [] | |
| for obj in objs: | |
| object_name = Path(obj.key).name | |
| # target an artifact for a specific job_id if provided, otherwise skip the download. | |
| if job_id is not None and str(job_id) not in object_name: | |
| continue | |
| found_one = True | |
| p = Path(Path(obj.key).name) | |
| print(f"Downloading {p}") | |
| s3_resource = get_s3_resource() | |
| bucket_obj = s3_resource.Bucket(GHA_ARTIFACTS_BUCKET) | |
| obj_data = bucket_obj.Object(obj.key).get() | |
| with open(p, "wb") as f: | |
| f.write(obj.get()["Body"].read()) | |
| f.write(obj_data["Body"].read()) | |
| paths.append(p) | |
| if not found_one: | |
| found_one = False | |
| paths = [] | |
| s3_resource = get_s3_resource() | |
| bucket_obj = s3_resource.Bucket(GHA_ARTIFACTS_BUCKET) | |
| for obj in objs: | |
| object_name = Path(obj.key).name | |
| if job_id is not None and str(job_id) not in object_name: | |
| continue | |
| found_one = True | |
| p = Path(Path(obj.key).name) | |
| print(f"Downloading {p}") | |
| obj_data = bucket_obj.Object(obj.key).get() | |
| with open(p, "wb") as f: | |
| f.write(obj_data["Body"].read()) | |
| paths.append(p) | |
| if not found_one: |
| json.dump(doc, body) | ||
| body.write("\n") | ||
|
|
||
| get_s3_resource().Object( | ||
| f"{bucket_name}", | ||
| f"{key}", | ||
| ).put( | ||
| s3_obj = get_s3_resource().Object(f"{bucket_name}", f"{key}") | ||
| s3_obj.put( | ||
| Body=gzip.compress(body.getvalue().encode()), | ||
| ContentEncoding="gzip", | ||
| ContentType="application/json", |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The StringIO object body is used inefficiently. It is written to and then immediately read from, which can be optimized by directly writing the compressed data to the S3 object.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| json.dump(doc, body) | |
| body.write("\n") | |
| get_s3_resource().Object( | |
| f"{bucket_name}", | |
| f"{key}", | |
| ).put( | |
| s3_obj = get_s3_resource().Object(f"{bucket_name}", f"{key}") | |
| s3_obj.put( | |
| Body=gzip.compress(body.getvalue().encode()), | |
| ContentEncoding="gzip", | |
| ContentType="application/json", | |
| compressed_body = gzip.compress('\n'.join(json.dumps(doc) for doc in docs).encode()) | |
| s3_obj = get_s3_resource().Object(f"{bucket_name}", f"{key}") | |
| s3_obj.put( | |
| Body=compressed_body, | |
| ContentEncoding="gzip", | |
| ContentType="application/json", | |
| ) |
| manually instead of using the `test-suite` XML tag because xmlrunner does | ||
| not produce reliable output for it. | ||
| """ | ||
|
|
||
| test_cases = sorted(test_cases, key=lambda x: (x.get("file", ""), x.get("classname", ""))) | ||
|
|
||
| def get_key(test_case: dict[str, Any]) -> Any: | ||
| return ( |
There was a problem hiding this comment.
Correctness: 🟠 [LangGraph v3] The sorting of test_cases by file and classname is redundant since get_key already handles grouping by these attributes. Remove the sorting to avoid unnecessary computation.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| manually instead of using the `test-suite` XML tag because xmlrunner does | |
| not produce reliable output for it. | |
| """ | |
| test_cases = sorted(test_cases, key=lambda x: (x.get("file", ""), x.get("classname", ""))) | |
| def get_key(test_case: dict[str, Any]) -> Any: | |
| return ( | |
| """Group test cases by classname, file, and job_id. We perform the aggregation | |
| manually instead of using the `test-suite` XML tag because xmlrunner does | |
| not produce reliable output for it. | |
| """ | |
| def get_key(test_case: dict[str, Any]) -> Any: | |
| return ( |
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
EntelligenceAI PR Summary
This PR improves code quality in stats upload tools through stylistic refactoring and deterministic test case ordering.
download_s3_artifacts(),upload_to_s3(), andread_from_s3()by extracting intermediate variablessummarize_test_cases()for consistent output