Make FileOutcomeReport outcome labels configurable - #1707
Conversation
Add label_overrides so non-add pipelines log the right verb. The image-embedding path now logs embedded=N instead of added=N. raise_if_all_failed() is unaffected; the overrides only touch log_summary(). Existing callers are unchanged (default empty). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthrough
ChangesOutcome label overrides
Estimated code review effort: 2 (Simple) | ~10 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62dad9fb18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Type label_overrides as Mapping (read-only) per Python guidelines - Move _label helper below public log_summary Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ Fast Track: checks did not pass
To run the guardrails locally, from Reflects |
|
/review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lightly_studio/tests/core/test_file_outcome_report.py (1)
147-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that overridden successful outcomes still suppress examples.
This test verifies the custom count label but not the corresponding no-examples contract. Add negative assertions for
a.jpgandExamples 'embedded'; the image-embedding path supplies this override.Suggested assertions
assert "embedded=1" in text assert "added=1" not in text + assert "a.jpg" not in text + assert "Examples 'embedded'" not in text🤖 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 `@lightly_studio/tests/core/test_file_outcome_report.py` around lines 147 - 161, Extend test_log_summary__label_overrides to assert that the overridden successful outcome suppresses its examples: verify caplog.text does not contain “a.jpg” or “Examples 'embedded'”, while preserving the existing custom-label and non-overridden count assertions.
🤖 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.
Nitpick comments:
In `@lightly_studio/tests/core/test_file_outcome_report.py`:
- Around line 147-161: Extend test_log_summary__label_overrides to assert that
the overridden successful outcome suppresses its examples: verify caplog.text
does not contain “a.jpg” or “Examples 'embedded'”, while preserving the existing
custom-label and non-overridden count assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e731462d-e7c1-4f4c-8d91-b07d57c6f504
📒 Files selected for processing (2)
lightly_studio/src/lightly_studio/core/file_outcome_report.pylightly_studio/tests/core/test_file_outcome_report.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lightly_studio/src/lightly_studio/core/file_outcome_report.py
2 participants
What
TLDR
So far the
FileOutcomeReport.log_summary()always produced logs likeadded=N,Example added paths. After this, this PR can be changed toembedded=N/Example embedded paths.Detailed
FileOutcomeReport.log_summary()was hard-coded to the add-images vocabulary (added=N,Example added paths), so pipelines that reuse the report logged misleading verbs — the image-embedding path reportedadded=Nwhen files are actually embedded.This adds a per-instance
label_overrides: dict[FileOutcome, str]that affects onlylog_summary(); any outcome absent from the dict falls back tooutcome.value. A small_label()helper routes both the counts line and the example-paths lines through the overrides.The image-embedding path (
embed_image_files_batched) now builds the report withlabel_overrides={FileOutcome.ADDED: "embedded"}, so it logsembedded=N/Example embedded paths.raise_if_all_failed()and its message are unchanged — they keep the fixed "missing"/"broken" wording. Existing callers (add-images, YOLO/VOC scans) are unaffected sincelabel_overridesdefaults to empty.The crop-embedding path is intentionally left out (under review on another branch); video is pending.
Testing
tests/core/test_file_outcome_report.py: default still logsadded=/Example added paths; anADDED: "embedded"override logsembedded=/Example embedded pathswhile other outcomes keep their default labels; a second override (MISSING) proves generality.make static-checksand affected tests pass.🤖 Generated with Claude Code
Summary by CodeRabbit
Summary