Show sample for each Level type on ROBOT report#134
Show sample for each Level type on ROBOT report#134anitacaron wants to merge 5 commits intomasterfrom
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR updates Changes
Assessment against linked issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 5
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
Line range hint
1-27: Overall impact on Makefile is minimal and aligned with objectives.The removal of the
truncate_reports_for_githubtarget is the only change in this Makefile. This modification doesn't affect the execution of other targets or introduce any new dependencies. The overall structure for building and managing the dashboard remains intact.Consider updating any documentation that might reference the removed
truncate_reports_for_githubtarget to reflect this change. This will help maintain consistency between the code and its documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Makefile (1 hunks)
- util/create_report_html.py (1 hunks)
- util/dashboard_config.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
util/dashboard_config.py (1)
78-79: Verify the impact of disabling GitHub post-processingThe lines for post-processing files for GitHub have been commented out. This change may affect how the dashboard files are handled after their generation.
To ensure this change doesn't negatively impact the workflow, please run the following script:
This script will help identify any potential issues arising from the removal of this functionality. Please review the results to ensure that no critical GitHub-related processing is being overlooked.
Consider the following points:
- If this change is intentional, update any documentation that might reference the GitHub post-processing step.
- Ensure that any downstream processes or scripts that may have depended on this post-processing step are updated accordingly.
- If this is a temporary change for debugging or testing purposes, consider using a feature flag instead of commenting out the code to make it easier to re-enable in the future.
✅ Verification successful
Verified: Disabling GitHub post-processing has no adverse effects
The commented-out lines for post-processing files for GitHub in
util/dashboard_config.pydo not impact the current functionality:
- The
truncate_reports_for_githubmake target does not exist in theMakefile, indicating that this functionality is not in use.- Other GitHub-related references found are confined to comments and documentation, posing no risk to the workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the truncate_reports_for_github make target # Test 1: Search for references to the make target in Python files echo "Searching for references to truncate_reports_for_github in Python files:" rg --type python "truncate_reports_for_github" # Test 2: Check if the make target still exists in the Makefile echo "Checking if the truncate_reports_for_github target exists in the Makefile:" rg --type make "^truncate_reports_for_github:" # Test 3: Look for any GitHub-related processing in other parts of the codebase echo "Searching for other GitHub-related processing:" rg --type python "github"Length of output: 600
Script:
#!/bin/bash # Description: Re-verify references to truncate_reports_for_github and GitHub-related processing # Test 1: Search for references to the make target in Python files echo "Searching for references to truncate_reports_for_github in Python files:" rg "truncate_reports_for_github" -- '*.py' # Test 2: Check if the truncate_reports_for_github target exists in the Makefile echo "Checking if the truncate_reports_for_github target exists in the Makefile:" rg "^truncate_reports_for_github:" Makefile # Test 3: Look for any GitHub-related processing in other parts of the codebase echo "Searching for other GitHub-related processing in Python files:" rg "github" -- '*.py'Length of output: 649
Script:
#!/bin/bash # Description: Re-verify references to truncate_reports_for_github and GitHub-related processing # Test 1: Search for references to the make target in Python files echo "Searching for references to truncate_reports_for_github in Python files:" rg "truncate_reports_for_github" --glob "*.py" # Test 2: Check if the truncate_reports_for_github target exists in the Makefile echo "Checking if the truncate_reports_for_github target exists in the Makefile:" rg "^truncate_reports_for_github:" Makefile # Test 3: Look for any GitHub-related processing in Python files echo "Searching for other GitHub-related processing in Python files:" rg "github" --glob "*.py"Length of output: 8422
Script:
#!/bin/bash # Description: Re-verify references to truncate_reports_for_github and GitHub-related processing # Test 1: Search for references to the make target in Python files echo "Searching for references to truncate_reports_for_github in Python files:" rg "truncate_reports_for_github" --glob "*.py" # Test 2: Check if the truncate_reports_for_github target exists in the Makefile echo "Checking if the truncate_reports_for_github target exists in the Makefile:" rg "^truncate_reports_for_github:" Makefile # Test 3: Look for any GitHub-related processing in Python files echo "Searching for other GitHub-related processing in Python files:" rg "github" --glob "*.py"Length of output: 8422
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
util/create_report_html.py (2)
65-68: Good implementation of column validation.This properly addresses the previous review comment about ensuring required columns exist.
84-89: Good implementation of structured exception handling.This properly addresses the previous review comment about using logging instead of print statements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
util/create_report_html.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: QC
🔇 Additional comments (1)
util/create_report_html.py (1)
5-5: LGTM! Good addition of structured logging.The logging configuration provides better error tracking with timestamps and log levels.
Also applies to: 13-14
| parser = argparse.ArgumentParser(description='Create a report HTML page') | ||
| parser.add_argument('report', | ||
| type=argparse.FileType('r'), | ||
| type=argparse.FileType('r+'), |
There was a problem hiding this comment.
Consider using a separate output file instead of overwriting the input.
Opening the report file in 'r+' mode and overwriting it at line 82 is risky. If an error occurs during processing, the original data could be lost.
Consider adding a separate output parameter for the filtered report:
parser.add_argument('report',
- type=argparse.FileType('r+'),
+ type=argparse.FileType('r'),
help='TSV report to convert to HTML')
+parser.add_argument('--output-report',
+ type=argparse.FileType('w'),
+ help='Output filtered report TSV file (optional)')Then modify line 82:
-if len(report_filtered) > args.limitlines:
- report_filtered.to_csv(args.report, sep="\t", index=False)
+if args.output_report and len(report) > args.limitlines:
+ report_filtered.to_csv(args.output_report, sep="\t", index=False)Also applies to: 82-82
🤖 Prompt for AI Agents
In util/create_report_html.py at line 21, the argument parser currently opens
the input report file in 'r+' mode, which risks overwriting the original file if
an error occurs. To fix this, add a new command-line argument for a separate
output file to write the filtered report. Then, at line 82, change the code to
write to this new output file instead of overwriting the input file. This
ensures the original input remains intact in case of processing errors.
| if len(report_filtered) > args.limitlines: | ||
| report_filtered.to_csv(args.report, sep="\t", index=False) |
There was a problem hiding this comment.
Remove or fix the illogical condition.
The condition len(report_filtered) > args.limitlines doesn't make sense after filtering. The filtered report should already be within the limit.
Either remove this block or change the logic to save the filtered report when the original report exceeds the limit:
-if len(report_filtered) > args.limitlines:
- report_filtered.to_csv(args.report, sep="\t", index=False)
+# Save filtered report if original exceeded limit
+if len(report) > args.limitlines and args.output_report:
+ report_filtered.to_csv(args.output_report, sep="\t", index=False)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In util/create_report_html.py around lines 81 to 82, the condition checking if
the filtered report length exceeds the limit is illogical since filtering should
already enforce the limit. Fix this by changing the condition to check if the
original report length exceeds args.limitlines before saving the filtered
report, or remove the condition entirely to always save the filtered report.
| report_filtered = report.groupby(by=["Level","Rule Name","Subject"])[ | ||
| ["Level", "Rule Name", "Subject", "Property", "Value"] | ||
| ].apply( | ||
| lambda x: x.sample(calculate_sample_size(x, rest)) | ||
| ).reset_index(drop=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the sampling more robust against edge cases.
The current implementation could fail if groups are empty or if sampling parameters are invalid.
Consider adding error handling:
# Get a sample of each Level type
-report_filtered = report.groupby(by=["Level","Rule Name","Subject"])[
- ["Level", "Rule Name", "Subject", "Property", "Value"]
-].apply(
- lambda x: x.sample(calculate_sample_size(x, rest))
-).reset_index(drop=True)
+def safe_sample(group, sample_size_func):
+ try:
+ n = sample_size_func(group)
+ if n > 0 and len(group) > 0:
+ return group.sample(min(n, len(group)))
+ return group
+ except Exception as e:
+ logging.warning(f"Sampling failed for group: {e}")
+ return group
+
+report_filtered = report.groupby(by=["Level","Rule Name","Subject"])[
+ ["Level", "Rule Name", "Subject", "Property", "Value"]
+].apply(
+ lambda x: safe_sample(x, calculate_sample_size)
+).reset_index(drop=True)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In util/create_report_html.py around lines 71 to 75, the current groupby
sampling can fail if groups are empty or sampling parameters are invalid. To fix
this, add error handling inside the lambda function to check if the group is
empty or if the sample size is valid before calling sample. If invalid, return
the group as is or an empty DataFrame to avoid exceptions during sampling.
| rest = args.limitlines - error_count_level["ERROR"] | ||
|
|
||
| # Calculate the sample number for each level based on group size | ||
| def calculate_sample_size(group, rest): | ||
| if group["Level"].iloc[0] == "ERROR": | ||
| return group.shape[0] | ||
|
|
||
| return min(group.shape[0], rest) | ||
|
|
There was a problem hiding this comment.
Fix the sampling logic to properly distribute remaining lines.
The current logic incorrectly uses the full rest value for each non-ERROR group, which could exceed the limit when multiple groups exist.
The sampling should distribute the remaining lines proportionally among non-ERROR levels:
-error_count_error = error_count_level.get("ERROR", 0)
-if error_count_error < args.limitlines:
- rest = args.limitlines - error_count_level["ERROR"]
-
- # Calculate the sample number for each level based on group size
- def calculate_sample_size(group, rest):
- if group["Level"].iloc[0] == "ERROR":
- return group.shape[0]
-
- return min(group.shape[0], rest)
+error_count_error = error_count_level.get("ERROR", 0)
+if error_count_error < args.limitlines:
+ rest = args.limitlines - error_count_error
+
+ # Calculate proportional sample sizes for non-ERROR levels
+ non_error_counts = {level: count for level, count in error_count_level.items() if level != "ERROR"}
+ total_non_error = sum(non_error_counts.values())
+
+ level_sample_sizes = {"ERROR": error_count_error}
+ if total_non_error > 0:
+ for level, count in non_error_counts.items():
+ # Proportionally distribute remaining lines
+ level_sample_sizes[level] = int(rest * count / total_non_error)
+
+ def calculate_sample_size(group):
+ level = group["Level"].iloc[0]
+ group_key = (level, group["Rule Name"].iloc[0], group["Subject"].iloc[0])
+ # Distribute level sample size among groups of that level
+ level_sample = level_sample_sizes.get(level, 0)
+ # This is a simplified approach - you may want to further distribute among groups
+ return min(group.shape[0], max(1, level_sample))Also, line 56 still uses direct dictionary access instead of .get():
-rest = args.limitlines - error_count_level["ERROR"]
+rest = args.limitlines - error_count_errorCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In util/create_report_html.py around lines 56 to 64, fix the sampling logic by
replacing the direct dictionary access on line 56 with .get() to avoid KeyError,
and update the calculate_sample_size function to distribute the remaining lines
proportionally among non-ERROR groups instead of using the full rest value for
each group. Compute the total size of non-ERROR groups, then allocate sample
sizes based on each group's proportion of that total, ensuring the sum does not
exceed the limit.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
21-27: Replace the retired recipe with a single pointer comment.Lines 21-27 leave obsolete Make logic in-place as commented code. Since truncation now happens in
util/create_report_html.py, this block is more likely to confuse than help.♻️ Suggested cleanup
-# Truncate potentially huge robot reports -# truncate_reports_for_github: -# $(eval REPORTS := $(wildcard dashboard/*/robot_report.tsv)) -# for REP in $(REPORTS); do \ -# touch $$REP; \ -# cat $$REP | head -$(REPORT_LENGTH_LIMIT) > $$REP.tmp; \ -# mv $$REP.tmp $$REP; \ -# done +# Report sampling/truncation is handled in util/create_report_html.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 21 - 27, The commented-out Make recipe truncate_reports_for_github (and related variables like REPORTS and REPORT_LENGTH_LIMIT) is obsolete and should be removed to avoid confusion; replace the entire commented block with a single pointer comment that directs readers to util/create_report_html.py where truncation now happens, e.g. "Truncation moved to util/create_report_html.py — see that script for current behavior." Locate the commented recipe by the recipe name truncate_reports_for_github in the Makefile and update that section accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 21-27: The commented-out Make recipe truncate_reports_for_github
(and related variables like REPORTS and REPORT_LENGTH_LIMIT) is obsolete and
should be removed to avoid confusion; replace the entire commented block with a
single pointer comment that directs readers to util/create_report_html.py where
truncation now happens, e.g. "Truncation moved to util/create_report_html.py —
see that script for current behavior." Locate the commented recipe by the recipe
name truncate_reports_for_github in the Makefile and update that section
accordingly.
1 participant
Fixes #133
Preview using CDAO as example
Summary by CodeRabbit
Bug Fixes
New Features
Chores