feat: Apply fix predictor at inference time with confidence scores#242
feat: Apply fix predictor at inference time with confidence scores#242IshanKale wants to merge 7 commits into
Conversation
|
🎉 Thank you @IshanKale for submitting a Pull Request! We're excited to review your contribution. Before Review✅ Ensure all CI checks pass ⚡ Want faster reviews and contributor support? Join our Discord community: 🔗 https://discord.gg/FcXuyw2Rs Maintainers and mentors are active there and can help resolve blockers quickly. Happy Contributing! 🚀 |
|
can you add the easy/medium/hard label as per the guidline of SSoC26 |
|
@IshanKale , Do Fix template |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
@IshanKale , Same issue! |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
|
can you review it now |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
There was a problem hiding this comment.
PR #242 Review — Fix Success Predictor at Inference Time
Verdict: 🔴 Changes Requested
Summary
The PR's stated scope is a 4-file backend change: add fix_confidence to the Fix model, create fix_predictor.py, wire it into /fix, and add unit tests. The actual diff is 47 files, +2973/−1441 lines — making this one of the largest PRs in the repository, touching nearly every backend endpoint, the entire frontend dashboard, multiple test suites, CI config, and docs. The core ML feature is functionally reasonable but cannot be safely approved in this state.
🔴 Blocking Issues
1. Massive Scope Creep — PR is Misrepresented
The description says 4 files changed. The actual diff touches 47 files across backend, frontend, CI, and docs. Undisclosed changes include:
| Area | Files | Net Lines |
|---|---|---|
| Frontend dashboard refactor | 20+ files | ~900 lines |
| Backend org-scan cancellation | main.py, db.py |
~400 lines |
Path traversal / safe_job_dir |
fs.py, main.py |
~80 lines |
| CI / PR template | 2 files | ~70 lines |
| Docs | ML_ARCHITECTURE.md |
+50 lines |
| Tests for unrelated features | 5 test files | ~311 lines |
None of these are mentioned in the PR description. This PR must be split before it can be reviewed. Approving it as-is means approving ~2800 lines of unreviewed code silently.
2. Broken Import — fp_predictor and ranker Are Live in Production Code
The current main.py on this branch contains:
from .ml.fp_predictor import predictor
from .ml.ranker import load_ranker, scoring_function
RANKER = load_ranker()And later:
await _apply_fp_predictor(findings)- These modules (
fp_predictor.py,ranker.py) exist on the branch but are not described anywhere in the PR. - If either is broken, misconfigured, or has a missing model file without a fallback, the entire backend process will crash at startup.
- The PR checklist for "graceful fallback when model file is absent" is unchecked — this may be why.
Caution
If fp_predictor or ranker do not have the same None-return graceful fallback as fix_predictor.py, this is a startup-crash regression.
3. requirements.txt Not Updated in This PR
fix_predictor.py adds hard imports of joblib and pandas at the top of the module — these are unconditional imports (not wrapped in try/except). If the packages are absent, the module will fail to import and crash startup.
joblib, pandas, and scikit-learn already exist in requirements.txt on main (pre-existing from other ML work), so this is not a new gap — but the PR checklist item "requirements.txt / package.json updated if new dependencies added" is unchecked. The author should confirm these were pre-existing, not accidentally omitted.
4. Test for predict_proba Path Is Dead Code
mock_model = MagicMock()
mock_model.predict.return_value = [0.85, 0.95]
if hasattr(mock_model, "predict_proba"):
del mock_model.predict_probaMagicMock auto-creates any attribute on access via __getattr__, so hasattr(mock_model, "predict_proba") will always return True — the del branch will always execute, and the predict_proba code path in fix_predictor.py is never actually tested. A dedicated test case with a mock that has predict_proba configured is needed.
🟡 Non-Blocking Issues (Must Fix Before Re-Review)
5. Module-Level Execution on Import
# Load model at startup
FIX_PREDICTOR_MODEL = load_model()This runs at module import time. If the filesystem is unavailable (e.g., during test setup, CI sandbox with restricted paths), it can produce unexpected side effects. The existing pattern in fp_predictor.py and ranker.py on this branch also does this via RANKER = load_ranker(). Consider a lazy-load or explicit startup event hook in FastAPI instead.
6. predict_confidence Is Synchronous and Called in Async Handler
# in /fix handler (async)
fixes = predict_confidence(fixes)If the model is large and predict_proba is CPU-intensive, this blocks the event loop. It should be wrapped with await run_in_threadpool(predict_confidence, fixes) — consistent with how the existing fp_predictor is handled in this same branch via _apply_fp_predictor.
7. Feature Sorting Mutates the Input Silently
predict_confidence both mutates each Fix object (sets fix_confidence) and returns a new sorted list. This dual-mutation pattern is confusing. The caller sees the sorted list, but the original list passed in has also had its objects mutated. Prefer returning new objects or being explicit about mutation.
8. Checklist Items Left Unchecked
Three checklist items are explicitly unchecked in the PR:
- New ML model falls back gracefully when model file is absent
- No new
console.erroror unhandled Python exceptions introduced -
requirements.txt/package.jsonupdated if new dependencies added
These should not be left unchecked at review time — they indicate the author is uncertain about their own PR.
✅ What Is Done Well
fix_predictor.pycorrectly handles the missing-model case by returningNoneand continuing.- The
load_model()fallback pattern (os.path.exists→joblib.load→except Exception → None) is solid. *.pklis correctly gitignored; the model file is not committed.- The
fix_confidence: Optional[float] = Nonefield addition toFixis backward-compatible (existing API consumers receivenull). - The basic test structure (mock
os.path.exists, mockjoblib.load) is correct for unit testing the loader.
Requested Actions
- Split the PR into at minimum: (a) this ML feature, (b) frontend dashboard refactor, (c) org-scan cancellation, (d) path traversal fixes. Each should be independently reviewable.
- Audit
fp_predictor.pyandranker.pyfor startup-crash risk; confirm both have the same graceful fallback asfix_predictor.py. - Fix the
predict_probatest — use a separate test class or configurespec=onMagicMockto prevent auto-attribute creation. - Wrap
predict_confidenceinrun_in_threadpoolin the/fixasync handler. - Check all three unchecked checklist items and confirm status before re-requesting review.
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
There was a problem hiding this comment.
PR #242 Re-Review — Updated Verdict: 🟡 Conditional Approve (1 Minor Concern Remains)
Blocking Issues — All Resolved ✅
| # | Original Issue | Status |
|---|---|---|
| 1 | Scope misrepresentation (47 files, PR said 4) | |
| 2 | fp_predictor / ranker startup crash risk |
✅ Fixed |
| 3 | Hard imports of joblib/pandas crash on missing deps |
✅ Fixed |
| 4 | predict_proba test dead code via MagicMock |
✅ Fixed |
Detailed Findings
✅ Issue #2 — fp_predictor & ranker Startup Crash Risk: RESOLVED
fp_predictor.pyuses a lazy-loading class (FalsePositivePredictor) — no module-level IO. It fails gracefully viais_ready = False.ranker.pynow hastry/exceptwithNonereturns at every failure path.main.pyloads the ranker viaRANKER = Noneat module level, then loads it inside@app.on_event("startup")withrun_in_threadpoolwrapped in atry/except(line 130–133). No startup crash possible.
✅ Issue #3 — Hard Imports: RESOLVED
fix_predictor.py lines 6–12 now wrap joblib and pandas in a try/except ImportError, setting FIX_PREDICTOR_DEPENDENCIES_AVAILABLE = False. All downstream paths check this flag before using the packages.
✅ Issue #4 — Dead predict_proba Test: RESOLVED
test_fix_predictor.py now has three separate test methods:
test_graceful_fallback_missing_model— tests the None-model pathtest_predict_confidence_and_sort— usesMagicMock(spec=["predict"]), which correctly preventspredict_probafrom existing on the mocktest_predict_confidence_with_predict_proba— dedicated test usingMagicMock(spec=["predict_proba"])with a realnumpy2-column array
✅ Issue #5 — Module-Level Execution on Import: RESOLVED
fix_predictor.py line 41: FIX_PREDICTOR_MODEL = None at import time. Actual loading is deferred to _get_model() (lazy-load on first call). No IO at import.
✅ Issue #6 — Synchronous predict_confidence in Async Handler: RESOLVED
main.py line 860:
fixes = await run_in_threadpool(predict_confidence, fixes)Correctly offloaded to threadpool. ✅
✅ Issue #7 — Input Mutation: RESOLVED
fix_predictor.py now uses fix.model_copy(update={...}) throughout — creates new Fix objects, never mutates the input list.
Remaining Concern: Scope Creep Still Present ⚠️
The PR is 50 files changed, +3051/−1449 lines. The split was not done. The diff still includes:
- Frontend dashboard refactor (~20 files)
- Theme/CSS changes
- Multiple new hooks and page changes
However, none of these frontend changes are broken or dangerous — they are additive, and the core ML feature itself is now correctly implemented and tested.
Final Verdict
🟡 Approve with Note — All technical blocking issues are resolved. The code is safe to ship. The only remaining concern is that the scope was never split as requested, meaning ~2,600 lines of frontend refactoring are going in under a "fix predictor" PR title. If your team has a policy on PR size/scope, flag it to the author for future PRs. If not, this is safe to approve as-is.
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
Linked issue
Closes #181
What this PR does
This PR implements and applies the fix success predictor at inference time within the backend. It loads the ML model (
fix_predictor.pkl) at startup and annotates each proposed fix returned by the/fixendpoint with afix_confidencescore. Proposed fixes are sorted descending by confidence, and a fallback mechanism is included to gracefully returnnullwithout crashing if the model is absent.Type of change
ML tier (if applicable)
Stack affected
Changes
Backend
fix_confidencefield to theFixPydantic model.predict_confidenceinto the/fixAPI handler.Frontend
New dependencies
Database / schema changes
Testing
How did you test this?
I created and ran the unit tests under the active virtual environment: