Feat(#36): 형태소·문자 기반 피싱 분류 모델 추가 및 성능 비교 (2/2) - #43
Merged
Conversation
📝 WalkthroughWalkthroughThe change adds Logistic Regression and Linear SVM phishing classifiers, validated comparison-artifact persistence, and a CLI-driven evaluation workflow. It also adds reproducibility reports, committed artifact metadata, tests, and pytest runtime-directory configuration. ChangesSMS model comparison
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SplitManifest
participant ComparisonRunner
participant ModelSet
participant ArtifactStore
participant ReportStore
SplitManifest->>ComparisonRunner: validate manifest and dataset fingerprint
ComparisonRunner->>ModelSet: train and evaluate three models
ModelSet->>ArtifactStore: save Logistic Regression and Linear SVM artifacts
ComparisonRunner->>ReportStore: write comparison reports
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
data_science/SMSModel/modeling/linear_svm.py (2)
33-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the whitespace-only line in
__init__.Line 34 contains trailing whitespace only. Most linters (for example Ruff
W293) flag this.♻️ Proposed cleanup
def __init__(self) -> None: - self.vectorizer = TfidfVectorizer(🤖 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/modeling/linear_svm.py` around lines 33 - 53, Remove the whitespace-only line immediately inside the __init__ method of the model class, leaving a blank line without trailing spaces while preserving the surrounding initialization logic.
65-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the DataFrame validation logic.
_validate_dataframehere is nearly identical toLogisticRegressionPhishingClassifier._validate_dataframe. Only the required text column and the error message prefix differ. A shared helper inmodeling/base.pywould keep the label rules in one place.🤖 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/modeling/linear_svm.py` around lines 65 - 115, Extract the shared DataFrame validation logic from LinearSVMClassifier._validate_dataframe and LogisticRegressionPhishingClassifier._validate_dataframe into a helper in modeling/base.py. Parameterize the required text column and error-message prefix, while keeping the existing label validation rules and both classifiers’ public behavior unchanged; update both _validate_dataframe methods to reuse the helper.tests/data_science/SMSModel/test_model_comparison.py (1)
160-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the overwrite guard.
_check_artifact_targetsis the main safeguard against destroying committed artifacts, and no test exercises it. Add a test that pointsLOGISTIC_ARTIFACT_DIRECTORYat atmp_pathcontainingmodel.joblib, then assertsFileExistsErrorwhenoverwrite_artifacts=Falseand no error when it isTrue.💚 Proposed test
def test_existing_artifacts_block_the_run_without_overwrite( monkeypatch: pytest.MonkeyPatch, tmp_path: Path, ) -> None: """기존 artifact가 있으면 --overwrite-artifacts 없이 중단합니다.""" artifact_directory = tmp_path / "logistic_regression" artifact_directory.mkdir() (artifact_directory / "model.joblib").write_bytes(b"") monkeypatch.setattr( comparison, "LOGISTIC_ARTIFACT_DIRECTORY", artifact_directory, ) monkeypatch.setattr( comparison, "LINEAR_SVM_ARTIFACT_DIRECTORY", tmp_path / "linear_svm", ) with pytest.raises(FileExistsError, match="already exist"): comparison._check_artifact_targets(overwrite_artifacts=False) comparison._check_artifact_targets(overwrite_artifacts=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 `@tests/data_science/SMSModel/test_model_comparison.py` around lines 160 - 203, Add a test for comparison._check_artifact_targets that creates model.joblib under a monkeypatched LOGISTIC_ARTIFACT_DIRECTORY, verifies FileExistsError when overwrite_artifacts=False, and verifies no error when it is True. Use tmp_path for isolated directories and patch LINEAR_SVM_ARTIFACT_DIRECTORY as needed to keep the check focused on the existing logistic artifact.data_science/SMSModel/run_model_comparison.py (4)
265-271: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations to the split parameters.
train_df,validation_df, andtest_dfhave no annotations, unlike every other function in this module.♻️ Proposed change
+import pandas as pddef _evaluate_models( *, models: list[BasePhishingClassifier], - train_df, - validation_df, - test_df, + train_df: pd.DataFrame, + validation_df: pd.DataFrame, + test_df: pd.DataFrame, ) -> list[ModelEvaluationResult]:🤖 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_model_comparison.py` around lines 265 - 271, Update the _evaluate_models function signature by adding the appropriate DataFrame type annotation to train_df, validation_df, and test_df, matching the annotation convention used by other functions in the module.
483-490: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe markdown slice depends on an undocumented layout.
Line 490 drops the first two lines of
render_model_evaluation_markdownoutput. If that function adds a subtitle or a leading blank line, this runner silently deletes a table header row. Match the title line explicitly instead.♻️ Proposed hardening
common_lines = common_markdown.splitlines() - # 첫 줄 "# Phishing Model Evaluation"과 바로 다음 빈 줄을 제외 - lines.extend(common_lines[2:]) + # 최상위 제목과 뒤따르는 빈 줄만 제거하고 표 부분을 재사용 + body_start = 0 + if common_lines and common_lines[0].startswith("# "): + body_start = 1 + while ( + body_start < len(common_lines) + and not common_lines[body_start].strip() + ): + body_start += 1 + lines.extend(common_lines[body_start:])🤖 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_model_comparison.py` around lines 483 - 490, Update the markdown reuse logic around render_model_evaluation_markdown so it locates the "# Phishing Model Evaluation" title line explicitly, skips that title and only its following blank line, then extends lines with the remaining content. Remove the fixed common_lines[2:] assumption while preserving the table header and subsequent markdown.
228-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
run_model_comparison.pyre-implements logic thatcomparison_artifacts.pyalready exports. The runner duplicates the artifact filenames, the SHA-256 helper, and the fingerprint format validation. The two copies can drift, and the runner then accepts input that the artifact module rejects.
data_science/SMSModel/run_model_comparison.py#L228-L262: importMODEL_FILENAMEandMETADATA_FILENAMEfromdata_science.SMSModel.modeling.comparison_artifactsinstead of the literals"model.joblib"and"metadata.json".data_science/SMSModel/run_model_comparison.py#L97-L108: remove the local_calculate_sha256and reuse the equivalent helper fromcomparison_artifacts.pyafter promoting it to a public name.data_science/SMSModel/run_model_comparison.py#L164-L181: replace the inline 64-character hex checks with_validate_dataset_fingerprintfromcomparison_artifacts.py, again after promoting it to a public name.🤖 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_model_comparison.py` around lines 228 - 262, Consolidate artifact constants and validation logic with comparison_artifacts.py: in data_science/SMSModel/run_model_comparison.py lines 228-262, import and use MODEL_FILENAME and METADATA_FILENAME instead of literal filenames; in lines 97-108, remove the local _calculate_sha256 and reuse the promoted public helper from comparison_artifacts.py; and in lines 164-181, replace inline fingerprint checks with the promoted public _validate_dataset_fingerprint helper. Update comparison_artifacts.py to expose the two helpers publicly while preserving their existing validation behavior.
659-682: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider saving the artifacts before the reports.
_save_run_reportsruns first. If_save_new_model_artifactsthen raisesFileExistsError, the committed reports describe a run whose artifacts were never written. Saving the artifacts first keeps the two outputs consistent.🤖 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_model_comparison.py` around lines 659 - 682, Reorder the final save operations so _save_new_model_artifacts completes before _save_run_reports. Keep the existing arguments and return results unchanged, ensuring reports are only committed after artifact creation succeeds.data_science/SMSModel/reports/model_evaluation/model_comparison/comparison_run.json (1)
62-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe Naive Bayes baseline is degenerate on this test split.
true_negativeis 0 andfalse_positiveis 56, so the model labels every test row as phishing at the selected threshold. Recall 1.0 is therefore not informative. Note this in the comparison report so readers do not read the baseline as a valid operating point.🤖 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/reports/model_evaluation/model_comparison/comparison_run.json` around lines 62 - 72, Update the comparison report’s Naive Bayes baseline entry around test_metrics to explicitly note that the test split is degenerate: all rows are classified as phishing at the selected threshold, with true_negative 0 and false_positive 56, making recall 1.0 uninformative. Preserve the existing metric values and add the note in the report’s appropriate metadata or explanatory field.data_science/SMSModel/artifacts/comparison/logistic_regression/metadata.json (1)
1-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommitted artifact metadata has no matching model payload. Both directories commit
metadata.json, but.gitignoreexcludesmodel.joblib.load_comparison_artifactrequires both files, so a fresh clone raisesFileNotFoundErrorfor either directory. Document the regeneration step so consumers know the metadata is a record, not a loadable artifact.
data_science/SMSModel/artifacts/comparison/logistic_regression/metadata.json#L1-L54: add a README indata_science/SMSModel/artifacts/comparison/that statesmodel.joblibis generated, and give the commandpython -m data_science.SMSModel.run_model_comparison --overwrite-artifacts.data_science/SMSModel/artifacts/comparison/linear_svm/metadata.json#L1-L54: cover this directory in the same README entry.🤖 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/comparison/logistic_regression/metadata.json` around lines 1 - 54, Add a README under data_science/SMSModel/artifacts/comparison/ documenting that model.joblib is generated and that both logistic_regression/metadata.json and linear_svm/metadata.json are metadata records rather than complete loadable artifacts until regeneration. Include the exact regeneration command: python -m data_science.SMSModel.run_model_comparison --overwrite-artifacts. Apply this documentation for data_science/SMSModel/artifacts/comparison/logistic_regression/metadata.json (lines 1-54) and data_science/SMSModel/artifacts/comparison/linear_svm/metadata.json (lines 1-54); neither metadata file requires a direct change.data_science/SMSModel/modeling/comparison_artifacts.py (1)
625-632: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe two
os.replacecalls are not atomic together.If the second
os.replacefails,model.joblibis new andmetadata.jsonis still the old file.load_comparison_artifactthen fails the checksum comparison, so the artifact directory becomes unloadable until the run is repeated. Consider replacingmetadata.jsonfirst, or removing both files if the second replace raises.♻️ Proposed hardening
- os.replace( - temporary_model_path, - model_path, - ) - os.replace( - temporary_metadata_path, - metadata_path, - ) + os.replace( + temporary_model_path, + model_path, + ) + try: + os.replace( + temporary_metadata_path, + metadata_path, + ) + except OSError: + # metadata 교체 실패 시 불일치 상태를 남기지 않도록 정리 + model_path.unlink(missing_ok=True) + metadata_path.unlink(missing_ok=True) + raise🤖 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/modeling/comparison_artifacts.py` around lines 625 - 632, Harden the replacement sequence around the two os.replace calls so a failure during the second replacement cannot leave mismatched model and metadata files. Replace metadata.json first, or, if the second replacement fails, remove both destination artifacts to avoid an unloadable mixed-version directory; preserve the existing successful replacement 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 `@tests/data_science/SMSModel/modeling/test_comparison_artifacts.py`:
- Around line 499-502: Update the pytest.raises call’s match argument in the
relevant test to use a raw regex string for the existing pattern, preserving its
alternatives and expected ValueError behavior.
---
Nitpick comments:
In
`@data_science/SMSModel/artifacts/comparison/logistic_regression/metadata.json`:
- Around line 1-54: Add a README under
data_science/SMSModel/artifacts/comparison/ documenting that model.joblib is
generated and that both logistic_regression/metadata.json and
linear_svm/metadata.json are metadata records rather than complete loadable
artifacts until regeneration. Include the exact regeneration command: python -m
data_science.SMSModel.run_model_comparison --overwrite-artifacts. Apply this
documentation for
data_science/SMSModel/artifacts/comparison/logistic_regression/metadata.json
(lines 1-54) and
data_science/SMSModel/artifacts/comparison/linear_svm/metadata.json (lines
1-54); neither metadata file requires a direct change.
In `@data_science/SMSModel/modeling/comparison_artifacts.py`:
- Around line 625-632: Harden the replacement sequence around the two os.replace
calls so a failure during the second replacement cannot leave mismatched model
and metadata files. Replace metadata.json first, or, if the second replacement
fails, remove both destination artifacts to avoid an unloadable mixed-version
directory; preserve the existing successful replacement behavior.
In `@data_science/SMSModel/modeling/linear_svm.py`:
- Around line 33-53: Remove the whitespace-only line immediately inside the
__init__ method of the model class, leaving a blank line without trailing spaces
while preserving the surrounding initialization logic.
- Around line 65-115: Extract the shared DataFrame validation logic from
LinearSVMClassifier._validate_dataframe and
LogisticRegressionPhishingClassifier._validate_dataframe into a helper in
modeling/base.py. Parameterize the required text column and error-message
prefix, while keeping the existing label validation rules and both classifiers’
public behavior unchanged; update both _validate_dataframe methods to reuse the
helper.
In
`@data_science/SMSModel/reports/model_evaluation/model_comparison/comparison_run.json`:
- Around line 62-72: Update the comparison report’s Naive Bayes baseline entry
around test_metrics to explicitly note that the test split is degenerate: all
rows are classified as phishing at the selected threshold, with true_negative 0
and false_positive 56, making recall 1.0 uninformative. Preserve the existing
metric values and add the note in the report’s appropriate metadata or
explanatory field.
In `@data_science/SMSModel/run_model_comparison.py`:
- Around line 265-271: Update the _evaluate_models function signature by adding
the appropriate DataFrame type annotation to train_df, validation_df, and
test_df, matching the annotation convention used by other functions in the
module.
- Around line 483-490: Update the markdown reuse logic around
render_model_evaluation_markdown so it locates the "# Phishing Model Evaluation"
title line explicitly, skips that title and only its following blank line, then
extends lines with the remaining content. Remove the fixed common_lines[2:]
assumption while preserving the table header and subsequent markdown.
- Around line 228-262: Consolidate artifact constants and validation logic with
comparison_artifacts.py: in data_science/SMSModel/run_model_comparison.py lines
228-262, import and use MODEL_FILENAME and METADATA_FILENAME instead of literal
filenames; in lines 97-108, remove the local _calculate_sha256 and reuse the
promoted public helper from comparison_artifacts.py; and in lines 164-181,
replace inline fingerprint checks with the promoted public
_validate_dataset_fingerprint helper. Update comparison_artifacts.py to expose
the two helpers publicly while preserving their existing validation behavior.
- Around line 659-682: Reorder the final save operations so
_save_new_model_artifacts completes before _save_run_reports. Keep the existing
arguments and return results unchanged, ensuring reports are only committed
after artifact creation succeeds.
In `@tests/data_science/SMSModel/test_model_comparison.py`:
- Around line 160-203: Add a test for comparison._check_artifact_targets that
creates model.joblib under a monkeypatched LOGISTIC_ARTIFACT_DIRECTORY, verifies
FileExistsError when overwrite_artifacts=False, and verifies no error when it is
True. Use tmp_path for isolated directories and patch
LINEAR_SVM_ARTIFACT_DIRECTORY as needed to keep the check focused on the
existing logistic artifact.
🪄 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: 1bb3ee0a-ed9a-4c44-81d5-26b55bd0b120
⛔ Files ignored due to path filters (1)
data_science/SMSModel/reports/model_evaluation/model_comparison/model_evaluation.csvis excluded by!**/*.csv
📒 Files selected for processing (19)
.gitignoredata_science/SMSModel/artifacts/comparison/linear_svm/metadata.jsondata_science/SMSModel/artifacts/comparison/linear_svm/model.joblibdata_science/SMSModel/artifacts/comparison/logistic_regression/metadata.jsondata_science/SMSModel/artifacts/comparison/logistic_regression/model.joblibdata_science/SMSModel/modeling/__init__.pydata_science/SMSModel/modeling/comparison_artifacts.pydata_science/SMSModel/modeling/linear_svm.pydata_science/SMSModel/modeling/logistic_regression.pydata_science/SMSModel/reports/model_evaluation/model_comparison/comparison_run.jsondata_science/SMSModel/reports/model_evaluation/model_comparison/comparison_run.mddata_science/SMSModel/reports/model_evaluation/model_comparison/model_evaluation.jsondata_science/SMSModel/reports/model_evaluation/model_comparison/model_evaluation.mddata_science/SMSModel/run_model_comparison.pypytest.initests/data_science/SMSModel/modeling/test_comparison_artifacts.pytests/data_science/SMSModel/modeling/test_linear_svm.pytests/data_science/SMSModel/modeling/test_logistic_regression.pytests/data_science/SMSModel/test_model_comparison.py
Comment on lines
+499
to
+502
| with pytest.raises( | ||
| ValueError, | ||
| match="not fitted|does not contain fitted classes", | ||
| ): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a raw regex literal.
Line 501 passes a regex containing | as a non-raw string. This produces Ruff RUF043. Change match to a raw string.
Proposed fix
- match="not fitted|does not contain fitted classes",
+ match=r"not fitted|does not contain fitted classes",📝 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.
Suggested change
| with pytest.raises( | |
| ValueError, | |
| match="not fitted|does not contain fitted classes", | |
| ): | |
| with pytest.raises( | |
| ValueError, | |
| match=r"not fitted|does not contain fitted classes", | |
| ): |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 501-501: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 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/modeling/test_comparison_artifacts.py` around
lines 499 - 502, Update the pytest.raises call’s match argument in the relevant
test to use a raw regex string for the existing pattern, preserving its
alternatives and expected ValueError behavior.
Source: Linters/SAST tools
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 개요
이 PR은 이슈 #36의 두 번째 작업
(2/2)으로, PR #38에서 구현한 재현 가능한 데이터 분할 및 공통 평가 파이프라인을 기반으로 신규 피싱 분류 모델을 추가하고 성능을 비교합니다.형태소 TF-IDF 기반 Logistic Regression과 문자 n-gram TF-IDF 기반 Linear SVM을 구현했습니다. 기존 Naive Bayes 모델을 포함한 세 모델을 동일한 train/validation/test 데이터로 학습·평가하고, 비교 보고서와 재사용 가능한 모델 artifact를 자동 생성하도록 구성했습니다.
기존 Naive Bayes 운영 artifact와 분석 API 동작은 변경하지 않습니다.
🔗 관련 이슈
🎯 주요 변경 사항
1. 형태소 TF-IDF Logistic Regression 모델 추가
normalize_text()전처리 사용class_weight="balanced"Logistic Regression 적용ScoreType.PROBABILITY형식으로 반환2. 문자 n-gram TF-IDF Linear SVM 모델 추가
text_norm입력 사용char_wb문자 n-gram TF-IDF 구성class_weight="balanced"LinearSVC 적용decision_function()결과를ScoreType.DECISION형식으로 반환3. 비교 모델 artifact 저장·로딩 기능 추가
4. 세 모델 통합 학습·평가 실행기 추가
sms_split_v1.csvmanifest 사용 강제5. 최종 모델 비교 결과
6. 생성 파일
비교 보고서:
reports/model_evaluation/model_comparison/model_evaluation.jsonreports/model_evaluation/model_comparison/model_evaluation.csvreports/model_evaluation/model_comparison/model_evaluation.mdreports/model_evaluation/model_comparison/comparison_run.jsonreports/model_evaluation/model_comparison/comparison_run.md비교 artifact:
artifacts/comparison/logistic_regression/model.joblibartifacts/comparison/logistic_regression/metadata.jsonartifacts/comparison/linear_svm/model.joblibartifacts/comparison/linear_svm/metadata.json📸 사진
✅ PR 체크리스트
uvicorn구동 또는 테스트 코드)를 통과했습니다.Summary by CodeRabbit
New Features
Tests
Chores