Conversation
|
elmartinj seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Rebase of the feat/improve-speed branch onto main surfaced several issues: - assign quantile columns one at a time: utilsforecast only supports single-column assignment for list values - accept a precomputed panel in the base Forecaster, Tafsut, and TimesFM 3.0 forecast paths - thread the optional panel argument through MultiModelForecasterMixin so custom models can opt in without breaking the type contract - compute the Chronos panel once and reuse it for dataset and finetuning - drop redundant panel assignments in TimesFM 1.0 / 2.5 - normalize string ds columns to datetime when building shared panels, matching the existing dataset behavior and the README quick example - add a download-free regression test for quantile-column assignment
44c69f1 to
a8deacb
Compare
Resolve timesfm.py import conflict by keeping shared PanelData support from #18 and maybe_convert_col_to_datetime from main. Co-authored-by: Cursor <cursoragent@cursor.com>
Merging this PR will improve performance by ×2.5
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | test_foundation_forecast_multi_model[5-models] |
967.3 ms | 174.2 ms | ×5.6 |
| ⚡ | Simulation | test_foundation_forecast_multi_model[3-models] |
579.8 ms | 145.6 ms | ×4 |
| ⚡ | Simulation | test_timeseries_dataset_from_df[small] |
10.8 ms | 5.2 ms | ×2.1 |
| ⚡ | Simulation | test_timeseries_dataset_from_df[large] |
180.3 ms | 107.4 ms | +67.86% |
| ⚡ | WallTime | test_model_forecast[moirai-1.1] |
1.6 s | 1.4 s | +15.39% |
| 🆕 | Simulation | test_assign_quantile_forecasts |
N/A | 4.6 ms | N/A |
| 🆕 | Simulation | test_process_panel_from_df_string_ds |
N/A | 225.4 ms | N/A |
| 🆕 | Simulation | test_process_panel_from_df[large] |
N/A | 104.4 ms | N/A |
| 🆕 | Simulation | test_process_panel_from_df[small] |
N/A | 4.7 ms | N/A |
| 🆕 | Simulation | test_timeseries_dataset_from_df_with_panel |
N/A | 42.5 µs | N/A |
| 🆕 | Simulation | test_timeseries_dataset_from_panel |
N/A | 35.2 µs | N/A |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing feat/improve-speed-refresh (df81538) with main (d6cc53f)
* feat: add panel and multi-model CodSpeed benchmarks Add large-panel dataset fixture, parametrize TimeSeriesDataset.from_df benchmarks, and FoundationForecast multi-model orchestration benchmarks to measure preprocessing speedups from shared panel processing (PR #18). Co-authored-by: Cursor <cursoragent@cursor.com> * fix: satisfy mypy list invariance in orchestration benchmark Annotate models as list[Forecaster] when constructing FoundationForecast. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Shared panel processing now passes panel to model forecast calls; update the FailingModel stub so the fallback path is exercised correctly. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
MultiModelForecasterMixin must remain compatible with custom or older forecasters that do not accept the panel parameter.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request reuses shared PanelData preprocessing across forecasters to reduce redundant work.
Changes:
- Adds shared panel processing and lazy dataset construction.
- Updates forecasters, model wrappers, and quantile handling.
- Adds tests, benchmarks, and CodSpeed coverage.
File summaries
| File | Summary |
|---|---|
tests/test_foundation_forecast.py |
Updates forecasting tests for shared panels. |
tests/helpers.py |
Updates panel-aware test helpers. |
tests/core/test_utils.py |
Tests shared panel processing. |
tests/core/test_model_weight_cache.py |
Maintains model weight cache coverage. |
tests/core/test_forecaster.py |
Tests panel helpers and quantile assignment. |
tests/benchmarks/test_panel.py |
Adds panel-processing benchmarks. |
tests/benchmarks/conftest.py |
Adds benchmark fixtures. |
foundationforecast/models/toto.py |
Adds optional shared-panel support. |
foundationforecast/models/tirex.py |
Adds optional shared-panel support. |
foundationforecast/models/timesfm.py |
Adds optional shared-panel support. |
foundationforecast/models/timegpt.py |
Adds optional shared-panel support. |
foundationforecast/models/tafsut.py |
Adds optional shared-panel support. |
foundationforecast/models/tabpfn.py |
Adds optional shared-panel support. |
foundationforecast/models/t0.py |
Adds optional shared-panel support. |
foundationforecast/models/sundial.py |
Adds optional shared-panel support. |
foundationforecast/models/patchtst_fm.py |
Adds optional shared-panel support. |
foundationforecast/models/flowstate.py |
Adds optional shared-panel support. |
foundationforecast/models/chronos.py |
Reuses panels for dataset and finetuning inputs. |
foundationforecast/core/utils.py |
Adds shared PanelData processing. |
foundationforecast/core/multi_model.py |
Shares panels across model calls. |
foundationforecast/core/gluonts_forecaster.py |
Reuses shared panels for GluonTS forecasts. |
foundationforecast/core/forecaster.py |
Adds panel support and quantile helpers. |
foundationforecast/core/__init__.py |
Exports new panel utilities. |
.github/workflows/codspeed.yml |
Runs panel benchmarks. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Inspect model forecast signatures before forwarding shared PanelData so custom legacy Forecaster subclasses and fallback paths remain compatible. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codspeedbot explain why performance improved |
Here's where the ×2.5 comes from. Flamegraphs aren't available for this repo's Simulation runs (the callgraph symbols failed to resolve on both the base and head runs), so I used the benchmark suite itself plus the diff — and in this case the new 1. Preprocessing went from per-model to once per call (the big one)The orchestration benchmarks scale linearly in the number of models, so you can read off the fixed and per-model costs directly:
The per-model cost dropped ~14×, and the fixed cost (~103 ms) matches the new 2. Dataset construction itself got cheaper
Worth being precise about the second point: for the benchmarked path that cost is deferred, not eliminated — the head number for 3. The WallTime gain on Moirai is a separate fix
SummaryThe speedup is structural, not micro-tuning: work that was O(models) — panel preprocessing — became O(1), and the remaining O(models) work is now cheap dataframe assembly. The new No code changes were needed for this request, so nothing was pushed to the branch. Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
There was a problem hiding this comment.
🔵 Needs a closer look
The broad core and model-wrapper changes, plus requested direct reuse coverage, warrant human review before approval.
Review details
Suppressed comments (1)
foundationforecast/core/multi_model.py:85
- The shared-panel path is the core optimization here, but the current orchestration tests do not verify it:
DatasetTouchingModelrebuilds a panel when the mixin omits one, so the benchmark still passes if this code regresses to per-model preprocessing. Please add a unit test that spies onprocess_panel_from_dfand asserts it is called once and that the samePanelDataobject is passed to each compatible model (and fallback).
if panel is None and attr == "forecast":
panel = process_panel_from_df(df)
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
This is a refreshed version of #2 (feat: use common syntaxis and improve speed), rebased onto current
main. The original commits by Azul are preserved on this branch; the final commit fixes what was blocking the old PR.What's in it:
PanelDataand reuse it across model calls instead of every wrapper re-doing sorting, datetime conversion, and dataset construction.panelthrough the baseForecaster.forecastsignature andMultiModelForecasterMixin.forecast, and update every built-in model wrapper (including Tafsut and TimesFM 3.0, which landed onmainafter the original PR).utilsforecast.assign_columnsrejects list values for multiple columns._ = panelstatements in the TimesFM 1.0 / 2.5 paths.Local verification: pre-commit (ruff, ruff-format, mypy, toml-sort) passes and the download-free test suite passes (78 passed, 6 skipped). Full model tests still need a CI run.
Related: #2