Skip to content

fix: reject a non-positive cross_validation period instead of looping forever - #2770

Open
NishchayMahor wants to merge 1 commit into
meta:mainfrom
NishchayMahor:fix/cross-validation-nonpositive-period
Open

NishchayMahor wants to merge 1 commit into
meta:mainfrom
NishchayMahor:fix/cross-validation-nonpositive-period

Conversation

@NishchayMahor

Copy link
Copy Markdown
Contributor

cross_validation never returns if period is zero or negative.

m.fit(df)
cross_validation(m, horizon="30 days", period="0 days")   # hangs

No error, no warning, no progress. The process sits at 100% CPU and grows memory until it is killed.

Why

generate_cutoffs walks backwards through the data one period at a time:

cutoff = df['ds'].max() - horizon
result = [cutoff]
while result[-1] >= min(df['ds']) + initial:
    cutoff -= period
    ...
    result.append(cutoff)

With period = 0 the subtraction is a no-op, so cutoff never moves and the loop condition never changes. result.append still runs on every pass, so memory climbs for as long as it is left alone. A negative period is worse: the cutoff walks away from the terminating condition.

Measured on 200 daily rows, horizon=30d, initial=60d, period=0:

iterations in 3s : 5,050
result list len  : 5,051   (one entry per iteration, never freed)
cutoff value     : unchanged from the first iteration

Nothing validates period. cross_validation does period = 0.5 * horizon if period is None else pd.Timedelta(period) and passes it straight through.

Only period has this property. I checked the other two durations against the same data, and both terminate even when zero or negative, so this fix is limited to period:

result
period = 0 never returns
period < 0 never returns
horizon = 0 20 cutoffs
horizon < 0 73 cutoffs
initial = 0 25 cutoffs
initial < 0 33 cutoffs

Fix

A guard in generate_cutoffs, immediately before the loop it protects.

I put it there rather than in cross_validation for two reasons. generate_cutoffs already raises its own ValueError for 'Less data than horizon.', so enforcing its own precondition is consistent with how it already behaves, and it covers callers who import it directly. The other reason is #2760: that PR is rewriting the exact lines in cross_validation where a parameter check would otherwise go, and there is no need for these two to collide.

One thing worth knowing

period defaults to 0.5 * horizon, so a zero or negative horizon with no explicit period now raises period must be positive. rather than hanging. The hang is gone, which is the point, but the message names a parameter the caller did not set. I left it alone rather than guess at how you would rather word it. Happy to change it.

Testing

test_cross_validation_nonpositive_period_raises, parametrised over "0 days" and "-7 days", going through cross_validation with the existing make_cross_validation_model() fake so it needs no fitted model.

The before/after here is unusual: without the fix the test does not fail, it hangs, so pytest-timeout is what turns it into an observable failure.

  • Reverting only diagnostics.py: 2 failed, both Failed: Timeout (>20.0s) from pytest-timeout, 40s of wall clock.
  • With the change: 2 passed in 0.05s.
  • test_diagnostics.py: 31 passed. The rest of python/prophet/tests: 124 passed, 2 skipped.

Not verified: test_plot.py cannot be collected in my environment because matplotlib is not installed, so it is excluded from that count. It fails to collect identically on a clean main, and this branch touches only diagnostics.py and its test. I also ran ruff and mypy over the touched files against both main and this branch and get identical counts, so nothing new is introduced, though I could not find a linter configured in CI to check against.

Not related: #2357 also reports a freeze, but @tcuongd diagnosed that one as cmdstanpy temp files during large searches, and it does not look like this.

@meta-cla meta-cla Bot added the CLA Signed label Oct 24, 2026
@NishchayMahor

Copy link
Copy Markdown
Contributor Author

Since this and #2760 both land on cross_validation's duration arguments, I checked how they interact rather than leaving it for whoever reviews second. They turn out to be independent, and both are needed.

I applied each branch separately and ran the same three inputs through cross_validation:

period main #2760 alone this PR alone both
"1 month" invalid unit abbreviation: month + Valid units are: W, D, h, min, s, ms, us, ns. invalid unit abbreviation: month + valid units
"0 days" hangs hangs period must be positive. period must be positive.
"-7 days" hangs hangs period must be positive. period must be positive.

The two do not overlap because they catch the value at different points. @Jason10191019999's _parse_timedelta wraps the call that turns a string into a Timedelta, so it only sees input pandas cannot parse. "0 days" parses fine — it is a perfectly valid Timedelta — so it goes straight past that helper and into the loop. Equally, "1 month" raises inside pd.Timedelta() well before generate_cutoffs runs, so the guard here never sees it.

They also do not conflict. I cherry-picked this commit onto #2760's branch: it applied with no conflict, and test_diagnostics.py is 34 passed on the combined branch (31 on main, plus 2 from here and 1 from #2760). Merge order does not matter and neither will need a rebase for the other.

That independence is why the guard here sits in generate_cutoffs rather than beside the parsing in cross_validation. #2760 is rewriting those exact three lines, and there was no reason for these to collide over them.

Worth being clear about the difference in kind, not quality: #2760 improves a message for input that already fails loudly and immediately. This one is about input that fails silently and indefinitely, which is the harder case to work out from the outside — a stuck process, full CPU, and no traceback to go on.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant