Test(#37): Bedrock Claude 하이브리드 성능·비용 평가 및 fallback 검증 (3/3) - #62
Conversation
|
Warning Review limit reached
Next review available in: 64 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR adds a hybrid SMS evaluation package with metrics, caching, three-mode execution, offline Bedrock workflows, report generation, validation, artifacts, documentation, and regression tests. ChangesHybrid evaluation workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🟡 Moderate · up to The PR adds hybrid evaluation and fallback behavior, but current results can misclassify stacking-derived outcomes and malformed latency values can break later offline evaluations; the documented fresh-clone workflow also lacks the required cache, and some telemetry is lost in published results. These concrete correctness and operational issues should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant CLI
participant ClaudeTestCache
participant HybridEvaluationRunner
participant HybridTextAnalyzer
participant ReportGenerator
CLI->>ClaudeTestCache: Load and update fixed-split predictions
CLI->>HybridEvaluationRunner: Run three evaluation modes
HybridEvaluationRunner->>HybridTextAnalyzer: Execute model and routing paths
HybridEvaluationRunner-->>CLI: Return evaluation records
CLI->>ReportGenerator: Build comparison outputs
ReportGenerator-->>CLI: Write JSON, Markdown, and CSV reports
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
pearseona
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (12)
tests/data_science/SMSModel/hybrid_evaluation/test_cache.py (2)
149-168: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for the "unexpected fingerprints" completeness branch.
require_completeindata_science/SMSModel/hybrid_evaluation/cache.py(lines 339-359) raises for two conditions: missing predictions and predictions outside the current test split. Only the missing branch is tested. The second branch protects the frozen test split from contamination by validation-split predictions, so it deserves a test.💚 Proposed additional test
+def test_require_complete_rejects_predictions_outside_split( + tmp_path, +) -> None: + cache = build_cache(tmp_path) + + cache.store_analysis( + text_fingerprint=FINGERPRINT_A, + analysis=successful_analysis(), + ) + cache.store_analysis( + text_fingerprint=FINGERPRINT_B, + analysis=successful_analysis(), + ) + + with pytest.raises( + RuntimeError, + match="outside", + ): + cache.require_complete({FINGERPRINT_A}) + +🤖 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 `@tests/data_science/SMSModel/hybrid_evaluation/test_cache.py` around lines 149 - 168, Add a test alongside test_require_complete_rejects_missing_predictions that stores a prediction for a fingerprint outside the requested set, then calls require_complete with the expected fingerprints and asserts it raises RuntimeError for the unexpected-fingerprint condition. Use existing cache builders and analysis fixtures, and verify the error identifies the completeness failure.
106-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parametrizing
regionin the metadata mismatch test.
build_cachepassesregion="us-east-1", but the mismatch test does not cover a changedregion. If the cache persists and validatesregion, add it to the parametrization. Ifregionis not validated, confirm that this is intended, because a cache collected in another region can be reused silently.🤖 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 `@tests/data_science/SMSModel/hybrid_evaluation/test_cache.py` around lines 106 - 146, The test_rejects_incompatible_cache parametrization should include a changed region case, using a value different from build_cache’s configured region, so load() must raise the existing metadata mismatch error. If cache metadata validation omits region, update that validation to reject mismatched regions before relying on the expanded test.data_science/SMSModel/README.md (1)
39-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a POSIX command form.
The new examples use PowerShell syntax and a Windows virtual environment path. The earlier section of this README uses
python -m data_science.SMSModel.train_sms. Add the equivalentpython -m ...invocation so that macOS and Linux contributors can reproduce the evaluation.🤖 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 `@data_science/SMSModel/README.md` around lines 39 - 59, Add POSIX-compatible evaluation commands to the README alongside the PowerShell examples, using the existing `python -m data_science.SMSModel...` invocation style and a Unix virtual-environment path. Include both offline evaluation/report generation and the `--collect` invocation so macOS and Linux users can reproduce the documented workflows.data_science/SMSModel/hybrid_evaluation/metrics.py (1)
186-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validating outcome element types.
calculate_cost_metricsraisesTypeErrorfor non-TokenUsageitems.calculate_operational_metricsdoes not apply the same check. A plain dict or a mock object passes silently and produces truthiness-based counts. Add the same guard for symmetry.♻️ Proposed guard
if len(outcomes) == 0: raise ValueError( "cannot calculate operational metrics " "from empty outcomes" ) + for outcome in outcomes: + if not isinstance(outcome, OperationalOutcome): + raise TypeError( + "outcomes must contain OperationalOutcome instances" + ) +🤖 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 `@data_science/SMSModel/hybrid_evaluation/metrics.py` around lines 186 - 242, Update calculate_operational_metrics to validate every outcomes element is an OperationalOutcome before calculating counts, raising TypeError for invalid values like dicts or mocks, matching the existing guard behavior in calculate_cost_metrics.data_science/SMSModel/artifacts/stacking/hybrid_policy.json (1)
5-16: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueIdentical hybrid policy metrics are stored in two artifacts. Both files now carry the same
f2,precision,recall,llm_call_count,llm_call_rate,normal_probability_max,phishing_probability_min,target_recall,target_recall_met, andvalidation_countvalues. If one file is regenerated without the other, the two artifacts disagree and readers cannot tell which one the evaluation applied.
data_science/SMSModel/artifacts/stacking/hybrid_policy.json#L5-L16: keep this file as the authoritative frozen policy, since the runner loads it.data_science/SMSModel/artifacts/stacking/metadata.json#L4-L15: replace the duplicated metric values with a reference tohybrid_policy.jsonand itsschema_version, or confirm that one generator writes both files in a single step.🤖 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 `@data_science/SMSModel/artifacts/stacking/hybrid_policy.json` around lines 5 - 16, The hybrid policy metrics are duplicated across two artifacts, allowing them to diverge. Keep data_science/SMSModel/artifacts/stacking/hybrid_policy.json lines 5-16 as the authoritative frozen policy loaded by the runner; update data_science/SMSModel/artifacts/stacking/metadata.json lines 4-15 to reference that policy and its schema_version, or ensure the generator writes both artifacts atomically in one step.tests/data_science/SMSModel/hybrid_evaluation/test_runner.py (1)
314-323: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for a non-string
text.
EvaluationSample.__post_init__raisesTypeErrorfor a non-stringtext. The parametrized cases only coverValueError. One extra case locks in both contracts.💚 Proposed addition
def test_rejects_invalid_samples(sample: tuple[str, str, str]) -> None: with pytest.raises(ValueError): EvaluationSample(*sample) + + +def test_rejects_non_string_text() -> None: + with pytest.raises(TypeError): + EvaluationSample("id", None, "normal")🤖 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 `@tests/data_science/SMSModel/hybrid_evaluation/test_runner.py` around lines 314 - 323, Add a parametrized sample with a non-string text value to test_rejects_invalid_samples and assert that constructing EvaluationSample raises TypeError for that case, while preserving the existing ValueError assertions for invalid string samples.data_science/SMSModel/run_hybrid_evaluation.py (1)
94-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
load_test_datatriggers split report regeneration.
split_datawrites the dataset split JSON and Markdown reports, and prints split summaries. Evaluation therefore rewrites training artifacts as a side effect. This can create unrelated diffs in the repository during an evaluation run.Consider loading the manifest directly, or document the side effect in
data_science/SMSModel/README.md.🤖 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 `@data_science/SMSModel/run_hybrid_evaluation.py` around lines 94 - 144, Update load_test_data to load and reconstruct the test split directly from SPLIT_MANIFEST_PATH instead of calling split_data, avoiding regeneration of split reports and other training artifacts during evaluation. Preserve the existing required-column, non-empty, unique-fingerprint, and supported-label validations.tests/analysis/text/test_stacking_analyzer.py (1)
354-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test duplicates the existing inference-failure test.
test_inference_error_returns_fail_safe_resultat Line 310 uses the same monkeypatching and only differs in the raised exception type. Parametrize the exception type on one test instead of keeping two near-identical bodies.🤖 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 `@tests/analysis/text/test_stacking_analyzer.py` around lines 354 - 381, Consolidate the duplicate stacking failure tests by parameterizing test_inference_error_returns_fail_safe_result with the relevant exception types, including ValueError. Remove test_feature_extraction_failure_makes_stacking_unavailable and keep the shared fail-safe assertions in the single parameterized test.data_science/SMSModel/hybrid_evaluation/cache.py (2)
136-140: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
entriesdeep-copies the whole cache on every access.The collection loop in
data_science/SMSModel/run_hybrid_evaluation.pyLines 286-288 readscache.entries[fingerprint]["available"]once per sample. Each read copies all stored entries, so collection is quadratic in split size. The current split is small, so this is not urgent. A small accessor avoids the copy.♻️ Proposed accessor
`@property` def entries(self) -> dict[str, dict[str, Any]]: """외부 변경을 막기 위해 복사본 반환""" return deepcopy(self._entries) + + def is_available(self, text_fingerprint: str) -> bool: + """단일 항목의 available 값만 조회""" + + entry = self._entries.get(text_fingerprint) + + return bool(entry and entry.get("available") is True)🤖 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 `@data_science/SMSModel/hybrid_evaluation/cache.py` around lines 136 - 140, Update the cache access pattern around the entries property in the cache class to provide a non-copying accessor for a single fingerprint, then use that accessor in the collection loop of run_hybrid_evaluation instead of repeatedly reading cache.entries. Preserve entries’ defensive deep-copy behavior for callers that need the full cache.
89-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShared helpers are copied across three modules.
_atomic_write_jsonand_optional_token_counteach exist in more than one file. The copies can drift, which is how the loader and the writer already disagree about non-finitelatency_ms. Place one implementation in a shared module inside thehybrid_evaluationpackage.
data_science/SMSModel/hybrid_evaluation/cache.py#L89-L115: move_atomic_write_jsonand_optional_token_countinto a new shared module, for examplehybrid_evaluation/io.py, and import them here.data_science/SMSModel/hybrid_evaluation/runner.py#L228-L231: delete the local_optional_token_countand import the shared one.data_science/SMSModel/run_hybrid_evaluation.py#L68-L92: delete the local_atomic_write_jsonand import the shared one.🤖 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 `@data_science/SMSModel/hybrid_evaluation/cache.py` around lines 89 - 115, Consolidate the duplicated _atomic_write_json and _optional_token_count helpers into a shared hybrid_evaluation/io.py module, preserving their existing behavior. In data_science/SMSModel/hybrid_evaluation/cache.py lines 89-115, move both helpers and import them from the shared module; in data_science/SMSModel/hybrid_evaluation/runner.py lines 228-231, delete the local _optional_token_count and import the shared helper; in data_science/SMSModel/run_hybrid_evaluation.py lines 68-92, delete the local _atomic_write_json and import the shared helper.data_science/SMSModel/hybrid_evaluation/runner.py (1)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLatency for cached runs mixes measured and recorded values.
The measured wall time covers only the cache read. Adding the recorded provider latency produces a hybrid number that is neither pure local time nor pure provider time. The sum stays comparable across modes, but a reader of the report cannot separate the two parts.
Consider storing the recorded provider latency in a separate field, and document the composition in the report.
🤖 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 `@data_science/SMSModel/hybrid_evaluation/runner.py` around lines 99 - 104, Separate the cache-read wall time from the recorded provider latency in the evaluation result around the latency calculation in runner.py: preserve the measured local latency, store cached_llm provider latency in its own field, and update the report output to label and document how the two values compose for cached runs.tests/analysis/text/test_hybrid_llm_failure_integration.py (1)
121-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
"secret" not in caplog.textis fragile.The assertion fails for any unrelated log text that contains the substring, for example a module path or the word "secrets". Assert on the full injected token 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 `@tests/analysis/text/test_hybrid_llm_failure_integration.py` around lines 121 - 124, Update the assertions in the hybrid LLM failure integration test to remove the broad `"secret" not in caplog.text` substring check and instead assert that the complete injected secret token is absent from the logs. Preserve the existing checks for the other sensitive values.
🤖 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 @.gitignore:
- Around line 21-23: Document the offline evaluation prerequisite near the
ignored LLM evaluation artifacts: a fresh clone must first run with --collect
and AWS access to generate llm_test_predictions.json, unless the cache is
committed outside the ignored path. Ensure the documentation makes clear that
--offline cannot be used successfully without an existing cache.
In `@app/analysis/text/hybrid_analyzer.py`:
- Around line 143-148: Add llm_usage, llm_latency_ms, and llm_from_cache to the
published TextAnalysisDetail contract, then update result_factory.py to
explicitly map these fields from the analyzer output into the RabbitMQ payload.
Preserve the direct /analyze response and HybridEvaluationRunner behavior.
In `@data_science/SMSModel/artifacts/stacking/hybrid_policy.json`:
- Around line 1-19: Add model provenance fields to the policy artifact
represented by the stacking policy JSON, including a stable model identifier or
artifact fingerprint and the model threshold used to derive the probability
bands. Update the policy loader’s validation to compare these values with the
current model and reject mismatches instead of accepting stale thresholds;
preserve the existing split metadata and threshold-selection fields.
In `@data_science/SMSModel/hybrid_evaluation/__init__.py`:
- Around line 35-60: Sort the `__all__` entries in
`data_science/SMSModel/hybrid_evaluation/__init__.py` alphabetically to satisfy
Ruff RUF022, preserving every currently exported symbol and changing only their
order.
In `@data_science/SMSModel/hybrid_evaluation/cache.py`:
- Around line 311-320: Update the latency_ms validation in store_analysis to
reject non-finite numeric values, matching the finiteness rule used by
_validate_cache_entry, while preserving boolean and negative-value rejection.
Optionally configure _atomic_write_json with allow_nan=False so invalid JSON
cannot be written.
In `@data_science/SMSModel/hybrid_evaluation/reporting.py`:
- Around line 39-40: Update pricing_as_of validation in the reporting
input-validation flow to parse an ISO date and reject values later than August
12, 2026; retain the existing non-empty validation for invalid or missing
values. Update affected fixtures and generated reports to use an effective date
on or before August 12, 2026, without labeling future dates as current pricing.
In `@data_science/SMSModel/hybrid_evaluation/runner.py`:
- Around line 185-194: Align stacking-derived label computation in _run_hybrid
with _stacking_to_public_result and the classifier threshold: use
is_suspected_phishing, or consistently apply the selected stacking threshold,
before calculating hybrid metrics. Ensure probabilities around 0.09–0.39 and
STACKING_FALLBACK produce the same phishing/normal classification as self-model
mode.
In `@data_science/SMSModel/run_hybrid_evaluation.py`:
- Around line 406-448: Update async_main and its caller main so offline
explicitly controls execution: preserve the --collect/--offline validation,
prevent collect_missing_predictions from running when offline is true, and
ensure offline mode uses only cached predictions without Bedrock calls. Remove
the derived offline = arguments.offline or not arguments.collect behavior that
makes the guard unreachable, and correct the cache.py comment to state that
offline mode avoids provider calls rather than importing the LLM module.
In `@tests/analysis/text/test_hybrid_llm_failure_integration.py`:
- Around line 108-124: Update
test_failure_logs_do_not_expose_message_or_credentials to capture logs from
DEBUG level by changing the caplog.at_level setting, so its existing privacy
assertions inspect INFO, WARNING, and ERROR records.
---
Nitpick comments:
In `@data_science/SMSModel/artifacts/stacking/hybrid_policy.json`:
- Around line 5-16: The hybrid policy metrics are duplicated across two
artifacts, allowing them to diverge. Keep
data_science/SMSModel/artifacts/stacking/hybrid_policy.json lines 5-16 as the
authoritative frozen policy loaded by the runner; update
data_science/SMSModel/artifacts/stacking/metadata.json lines 4-15 to reference
that policy and its schema_version, or ensure the generator writes both
artifacts atomically in one step.
In `@data_science/SMSModel/hybrid_evaluation/cache.py`:
- Around line 136-140: Update the cache access pattern around the entries
property in the cache class to provide a non-copying accessor for a single
fingerprint, then use that accessor in the collection loop of
run_hybrid_evaluation instead of repeatedly reading cache.entries. Preserve
entries’ defensive deep-copy behavior for callers that need the full cache.
- Around line 89-115: Consolidate the duplicated _atomic_write_json and
_optional_token_count helpers into a shared hybrid_evaluation/io.py module,
preserving their existing behavior. In
data_science/SMSModel/hybrid_evaluation/cache.py lines 89-115, move both helpers
and import them from the shared module; in
data_science/SMSModel/hybrid_evaluation/runner.py lines 228-231, delete the
local _optional_token_count and import the shared helper; in
data_science/SMSModel/run_hybrid_evaluation.py lines 68-92, delete the local
_atomic_write_json and import the shared helper.
In `@data_science/SMSModel/hybrid_evaluation/metrics.py`:
- Around line 186-242: Update calculate_operational_metrics to validate every
outcomes element is an OperationalOutcome before calculating counts, raising
TypeError for invalid values like dicts or mocks, matching the existing guard
behavior in calculate_cost_metrics.
In `@data_science/SMSModel/hybrid_evaluation/runner.py`:
- Around line 99-104: Separate the cache-read wall time from the recorded
provider latency in the evaluation result around the latency calculation in
runner.py: preserve the measured local latency, store cached_llm provider
latency in its own field, and update the report output to label and document how
the two values compose for cached runs.
In `@data_science/SMSModel/README.md`:
- Around line 39-59: Add POSIX-compatible evaluation commands to the README
alongside the PowerShell examples, using the existing `python -m
data_science.SMSModel...` invocation style and a Unix virtual-environment path.
Include both offline evaluation/report generation and the `--collect` invocation
so macOS and Linux users can reproduce the documented workflows.
In `@data_science/SMSModel/run_hybrid_evaluation.py`:
- Around line 94-144: Update load_test_data to load and reconstruct the test
split directly from SPLIT_MANIFEST_PATH instead of calling split_data, avoiding
regeneration of split reports and other training artifacts during evaluation.
Preserve the existing required-column, non-empty, unique-fingerprint, and
supported-label validations.
In `@tests/analysis/text/test_hybrid_llm_failure_integration.py`:
- Around line 121-124: Update the assertions in the hybrid LLM failure
integration test to remove the broad `"secret" not in caplog.text` substring
check and instead assert that the complete injected secret token is absent from
the logs. Preserve the existing checks for the other sensitive values.
In `@tests/analysis/text/test_stacking_analyzer.py`:
- Around line 354-381: Consolidate the duplicate stacking failure tests by
parameterizing test_inference_error_returns_fail_safe_result with the relevant
exception types, including ValueError. Remove
test_feature_extraction_failure_makes_stacking_unavailable and keep the shared
fail-safe assertions in the single parameterized test.
In `@tests/data_science/SMSModel/hybrid_evaluation/test_cache.py`:
- Around line 149-168: Add a test alongside
test_require_complete_rejects_missing_predictions that stores a prediction for a
fingerprint outside the requested set, then calls require_complete with the
expected fingerprints and asserts it raises RuntimeError for the
unexpected-fingerprint condition. Use existing cache builders and analysis
fixtures, and verify the error identifies the completeness failure.
- Around line 106-146: The test_rejects_incompatible_cache parametrization
should include a changed region case, using a value different from build_cache’s
configured region, so load() must raise the existing metadata mismatch error. If
cache metadata validation omits region, update that validation to reject
mismatched regions before relying on the expanded test.
In `@tests/data_science/SMSModel/hybrid_evaluation/test_runner.py`:
- Around line 314-323: Add a parametrized sample with a non-string text value to
test_rejects_invalid_samples and assert that constructing EvaluationSample
raises TypeError for that case, while preserving the existing ValueError
assertions for invalid string samples.
🪄 Autofix
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 Plus
Run ID: c1063e8b-5f63-4adb-ae8c-ec69726037c9
⛔ Files ignored due to path filters (1)
data_science/SMSModel/reports/hybrid_evaluation/comparison_report.csvis excluded by!**/*.csv
📒 Files selected for processing (24)
.gitignoreapp/analysis/text/hybrid_analyzer.pydata_science/SMSModel/README.mddata_science/SMSModel/artifacts/stacking/hybrid_policy.jsondata_science/SMSModel/artifacts/stacking/metadata.jsondata_science/SMSModel/generate_hybrid_evaluation_report.pydata_science/SMSModel/hybrid_evaluation/__init__.pydata_science/SMSModel/hybrid_evaluation/cache.pydata_science/SMSModel/hybrid_evaluation/metrics.pydata_science/SMSModel/hybrid_evaluation/models.pydata_science/SMSModel/hybrid_evaluation/reporting.pydata_science/SMSModel/hybrid_evaluation/runner.pydata_science/SMSModel/reports/hybrid_evaluation/comparison_report.jsondata_science/SMSModel/reports/hybrid_evaluation/comparison_report.mddata_science/SMSModel/reports/hybrid_evaluation/evaluation_records.jsondata_science/SMSModel/run_hybrid_evaluation.pytests/analysis/test_hybrid_policy.pytests/analysis/test_router.pytests/analysis/text/test_hybrid_llm_failure_integration.pytests/analysis/text/test_stacking_analyzer.pytests/data_science/SMSModel/hybrid_evaluation/test_cache.pytests/data_science/SMSModel/hybrid_evaluation/test_metrics.pytests/data_science/SMSModel/hybrid_evaluation/test_reporting.pytests/data_science/SMSModel/hybrid_evaluation/test_runner.py
| "llm_usage": { | ||
| "input_tokens": None, | ||
| "output_tokens": None, | ||
| }, | ||
| "llm_latency_ms": None, | ||
| "llm_from_cache": False, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how text analysis fields reach the API response and whether legacy aliases cover the new keys.
rg -n -C4 'llm_usage|llm_latency_ms|llm_from_cache' app tests
# Inspect the legacy alias helper.
ast-grep run --pattern 'def _with_legacy_aliases($$$):
$$$' --lang python app/analysis/text/hybrid_analyzer.py
# Locate response schemas for text analysis.
rg -n -C4 'llm_called|gemini_called' app --glob '!**/text/hybrid_analyzer.py'Repository: SafeFam/SafeFam_AI
Length of output: 7022
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- result factory ---'
sed -n '100,215p' app/infrastructure/rabbitmq/result_factory.py
printf '%s\n' '--- response model/schema candidates ---'
rg -n -C3 'TextAnalysis|llmProvider|llmModel|geminiCalled|decisionSource|routingReason|llmCalled' app tests --glob '*.py'
printf '%s\n' '--- evaluation token extraction ---'
rg -n -C5 'input_tokens|output_tokens|llm_usage|usage' app tests --glob '*.py'Repository: SafeFam/SafeFam_AI
Length of output: 48072
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- TextAnalysisDetail schema ---'
sed -n '104,148p' app/infrastructure/rabbitmq/schemas.py
printf '%s\n' '--- evaluation runner implementation ---'
sed -n '1,240p' app/data_science/SMSModel/hybrid_evaluation/runner.py 2>/dev/null || true
sed -n '1,240p' tests/data_science/SMSModel/hybrid_evaluation/test_runner.py
printf '%s\n' '--- runner and analyzer call sites ---'
rg -n -C5 'run_sample|run_batch|hybrid_analyzer|input_tokens|output_tokens|llm_usage' app tests/data_science --glob '*.py'
printf '%s\n' '--- serialization configuration ---'
rg -n -C4 'model_dump|model_config|extra=|AnalysisResultPayload|textAnalysis' app/infrastructure app --glob '*.py'Repository: SafeFam/SafeFam_AI
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- evaluation source files ---'
git ls-files | grep -E '(^|/)(data_science|hybrid_evaluation)(/|.*\.py$)' | head -80
printf '%s\n' '--- token extraction definitions ---'
rg -n -C8 'class HybridEvaluationRunner|def evaluate_one|input_tokens|output_tokens|llm_usage|llm_analyzer' . --glob '*.py' --glob '!tests/**'
printf '%s\n' '--- direct API mapping tests ---'
sed -n '1,190p' tests/infrastructure/rabbitmq/test_result_factory.py
printf '%s\n' '--- package exports ---'
find . -path '*hybrid_evaluation*' -maxdepth 8 -type f -name '*.py' -printRepository: SafeFam/SafeFam_AI
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- analysis response schemas ---'
rg -n -C4 'class .*Response|text_analysis|textAnalysis|llm' app/analysis/schemas.py app/api app --glob '*.py' | head -240
printf '%s\n' '--- runner token extraction and record construction ---'
sed -n '82,225p' data_science/SMSModel/hybrid_evaluation/runner.py
printf '%s\n' '--- result factory telemetry assertions ---'
rg -n -C5 'TextAnalysisDetail|llmCalled|llmProvider|decisionSource|model_dump|payload' tests/infrastructure/rabbitmq/test_result_factory.pyRepository: SafeFam/SafeFam_AI
Length of output: 26356
🏁 Script executed:
#!/bin/bash
set -e
rg -n -C6 'SmishingAnalysisResponse|text_analysis|build_payload|AnalysisResultEventFactory|analyze\(' app --glob '*.py' | head -320Repository: SafeFam/SafeFam_AI
Length of output: 18670
Map telemetry into the published result contract.
The direct /analyze response forwards text_analysis as a dictionary. The RabbitMQ result contract does not. TextAnalysisDetail forbids extra fields, and result_factory.py does not map llm_usage, llm_latency_ms, or llm_from_cache; the published payload drops them. Add explicit contract fields and mappings. HybridEvaluationRunner reads the analyzer output directly, so this omission does not cause its token values to become None.
🤖 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 `@app/analysis/text/hybrid_analyzer.py` around lines 143 - 148, Add llm_usage,
llm_latency_ms, and llm_from_cache to the published TextAnalysisDetail contract,
then update result_factory.py to explicitly map these fields from the analyzer
output into the RabbitMQ payload. Preserve the direct /analyze response and
HybridEvaluationRunner behavior.
| { | ||
| "created_at": "2026-08-12T15:08:55.703054+00:00", | ||
| "llm_phishing_score": 40, | ||
| "schema_version": 1, | ||
| "selection": { | ||
| "f2": 0.9544159544159544, | ||
| "llm_call_count": 51, | ||
| "llm_call_rate": 0.4146341463414634, | ||
| "normal_probability_max": 0.08716666259958943, | ||
| "phishing_probability_min": 0.59, | ||
| "precision": 0.8072289156626506, | ||
| "recall": 1.0, | ||
| "target_recall": 0.95, | ||
| "target_recall_met": true, | ||
| "validation_count": 123 | ||
| }, | ||
| "source_split": "validation", | ||
| "split_manifest": "sms_split_v1.csv" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record the model identity that these thresholds depend on.
normal_probability_max and phishing_probability_min are probability bands of one specific trained stacking model. The file records split_manifest and source_split, but no model identifier, model threshold, or artifact fingerprint. After a retrain, the frozen policy stays syntactically valid and the loader accepts stale thresholds silently.
Add the model provenance so that the loader can reject a policy that does not match the current model.
🛡️ Proposed provenance fields
"source_split": "validation",
- "split_manifest": "sms_split_v1.csv"
+ "split_manifest": "sms_split_v1.csv",
+ "model_name": "stacking_phishing_classifier",
+ "model_threshold": 0.08844759684377157
}🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 8-8: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
🤖 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 `@data_science/SMSModel/artifacts/stacking/hybrid_policy.json` around lines 1 -
19, Add model provenance fields to the policy artifact represented by the
stacking policy JSON, including a stable model identifier or artifact
fingerprint and the model threshold used to derive the probability bands. Update
the policy loader’s validation to compare these values with the current model
and reject mismatches instead of accepting stale thresholds; preserve the
existing split metadata and threshold-selection fields.
1 participant
📝 개요
고정된 test split을 기준으로 Stacking-Only, Claude-only, Hybrid 분석 방식을 동일한 조건에서 비교 평가했습니다.
Validation split에서 선정한 하이브리드 임계값을 고정해 test 데이터 재조정을 방지했으며, 정확도뿐 아니라 LLM 호출률, 지연시간, 토큰 사용량 및 비용을 함께 측정했습니다.
평가 결과 Hybrid 방식은 Recall 1.0을 유지하면서 Stacking-only 대비 F2를 개선했고, Calude-only 대비 LLM 호출률과 평균 비용 및 지연시간을 절감했습니다.
🔗 관련 이슈
🎯 주요 변경 사항
hybrid_policy.json에 저장하고 test 평가 시 고정해서 사용하도록 구성했습니다.gemini_*응답 필드의 하위 호환 alias를 유지했습니다.data_science/SMSModel/README.md에 문서화했습니다.재현 방법
✅ PR 체크리스트
uvicorn구동 또는 테스트 코드)를 통과했습니다.Summary by CodeRabbit
New Features
Documentation
Bug Fixes