fix(pandas-postprocessing): handle prophet errors and validate minimum data points for forecast#41180
fix(pandas-postprocessing): handle prophet errors and validate minimum data points for forecast#41180yousoph wants to merge 3 commits into
Conversation
…r prophet forecast - Add try/except around model.fit() and model.predict() in _prophet_fit_and_predict, re-raising prophet errors as InvalidPostProcessingError with a user-readable message - Add minimum 2 data points validation in prophet() before fitting, which is prophet's hard requirement; sparse daily data (e.g. Vehicle Sales dataset at Day granularity) could have only 1 row and trigger an unhandled 500 error - Update deprecated pandas frequency aliases: 'S'→'s', 'H'→'h', and add a runtime version check so 'M'/'Q'/'A' map to 'ME'/'QE'/'YE' on pandas>=2.2 (the new aliases are invalid on pinned pandas 2.1.4) - Add tests for insufficient data and prophet fitting errors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #16a87bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| ) | ||
|
|
||
|
|
||
| def test_prophet_insufficient_data(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test function (for example -> None) to satisfy the rule requiring fully typed new Python functions. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python function and it has no return type annotation. The custom rule requires new Python code to be fully typed, so omitting -> None is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/pandas_postprocessing/test_prophet.py
**Line:** 190:190
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test function (for example `-> None`) to satisfy the rule requiring fully typed new Python functions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Declining — no existing test functions in this file (or in the broader test suite) use -> None annotations or docstrings. Adding them only to the two new functions would be inconsistent with the established pattern here.
| ) | ||
|
|
||
|
|
||
| def test_prophet_fit_error(): |
There was a problem hiding this comment.
Suggestion: Add a concise docstring under this new function definition to describe the expected error-handling behavior. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python function and it has no docstring. That violates the rule requiring new functions and classes to include docstrings.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/pandas_postprocessing/test_prophet.py
**Line:** 206:206
**Comment:**
*Custom Rule: Add a concise docstring under this new function definition to describe the expected error-handling behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Declining — no existing test functions in this file use docstrings, and adding one only to the new functions would be inconsistent. Test name is self-describing.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not require docstrings for new test functions when the surrounding test file uses self-describing test names and existing tests in that file omit docstrings; keep the style consistent within the file.
Applied to:
tests/unit_tests/pandas_postprocessing/**
💡 To manage or update this instruction, visit: CodeAnt AI Settings
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41180 +/- ##
==========================================
+ Coverage 63.73% 64.30% +0.57%
==========================================
Files 2651 2651
Lines 144737 144753 +16
Branches 33402 33403 +1
==========================================
+ Hits 92242 93079 +837
+ Misses 50827 50004 -823
- Partials 1668 1670 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add noqa: BLE001 to broad exception catch (required since Prophet can raise a mix of ValueError/RuntimeError), and collapse single-argument raise to one line per ruff-format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in tests - Add noqa: C901 to prophet() — the new data-point guard legitimately adds one branch, pushing cyclomatic complexity to 11; suppressing is correct here - Wrap long pytest.raises() call in test_prophet_fit_error to stay under 88 chars Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #863d21Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
Two related bugs in the prophet/forecast post-processing pipeline caused any forecast chart to surface an unhandled 500 error when data conditions were unfavourable — most visibly when a high-granularity time grain (e.g. Day) was used with a dataset that had only a single data point in the selected range.
Fix 1 — minimum data-point guard (
prophet.py):Prophet hard-requires ≥ 2 data points to fit a model. The old code had no pre-flight check; Prophet raised a bare
ValueErrorthat propagated as an unhandled 500. A clearInvalidPostProcessingError("Forecast requires at least 2 data points")is now raised before any prophet call.Fix 2 — wrap fit/predict in try/except (
prophet.py):Even when enough data exists, Prophet can raise for other reasons (bad data, convergence failures, etc.). The
model.fit(),model.make_future_dataframe(), andmodel.predict()calls are now wrapped in atry/except Exceptionthat re-raises asInvalidPostProcessingErrorwith the original message, giving the user a readable error instead of a 500.Fix 3 — update deprecated pandas frequency aliases (
utils.py):PROPHET_TIME_GRAIN_MAPused aliases that are deprecated in pandas 2.2+ ("S","H","M","Q","A"):"S"→"s","H"→"h"(safe on all supported pandas versions)"M"/"Q"/"A"→"ME"/"QE"/"YE"when pandas ≥ 2.2, old aliases otherwise (the new names raiseValueErroron the currently pinned pandas 2.1.4)BEFORE/AFTER
Before: Enabling forecast with sparse daily data → unhandled
ValueErrorfrom Prophet → HTTP 500, generic error shown in the chart.After:
InvalidPostProcessingError("Forecast requires at least 2 data points")→ HTTP 422 with a user-readable message.TESTING INSTRUCTIONS
New tests:
test_prophet_insufficient_data— verifies the < 2 data points guard raises correctlytest_prophet_fit_error— verifies prophet fitting errors surface asInvalidPostProcessingErrorAll 11 prophet tests pass.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code