Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
rusackas merged PR #43205: URL: https://github.com/apache/superset/pull/43205 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
SEPURI-SAI-KRISHNA commented on PR #43205: URL: https://github.com/apache/superset/pull/43205#issuecomment-5450511249 Gentle nudge on this one — it's been open since Aug 15 without a first review. It's the smallest change in this area I have open: +54/-0 across two files, 46 of which are tests. No behaviour change beyond the added mappings. `PROPHET_TIME_GRAIN_MAP` holds 16 keys against 19 built-in time grains, so `PT5S`, `PT30S` and `PT6H` pass schema validation and then fail inside `prophet()`. This brings the map to 21 keys, making it a superset of the built-ins. That last part also unblocks #43356: deriving the published `time_grain` choices from the map is only safe once the map covers every built-in grain, otherwise those three silently drop out of the spec. CI is green (62 passing). No urgency — happy to rebase or split if that helps. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
SEPURI-SAI-KRISHNA commented on code in PR #43205:
URL: https://github.com/apache/superset/pull/43205#discussion_r3818875556
##
superset/utils/pandas_postprocessing/utils.py:
##
@@ -86,16 +86,24 @@
PROPHET_TIME_GRAIN_MAP: dict[str, str] = {
Review Comment:
Opened as #43356.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
SEPURI-SAI-KRISHNA commented on code in PR #43205:
URL: https://github.com/apache/superset/pull/43205#discussion_r3788672001
##
superset/utils/pandas_postprocessing/utils.py:
##
@@ -86,16 +86,24 @@
PROPHET_TIME_GRAIN_MAP: dict[str, str] = {
Review Comment:
Correct, and pre-existing, get_time_grain_choices() merges
TIME_GRAIN_ADDONS, so operator-defined grains are advertised but unmapped. That
can't be fixed by extending a static map, since addon keys are arbitrary
ISO-8601 durations; it needs a general duration→pandas-frequency converter. Out
of scope here, where the concern is the built-in grains Superset ships and
engine specs already offer. Happy to open a follow-up issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
codeant-ai-for-open-source[bot] commented on code in PR #43205:
URL: https://github.com/apache/superset/pull/43205#discussion_r3788510024
##
superset/utils/pandas_postprocessing/utils.py:
##
@@ -86,16 +86,24 @@
PROPHET_TIME_GRAIN_MAP: dict[str, str] = {
Review Comment:
**Suggestion:** The mapping still contains only built-in `TimeGrain` values,
while `ChartDataProphetOptionsSchema` also accepts keys from
`TIME_GRAIN_ADDONS`. A configured custom grain will pass schema validation and
then fail in `prophet()` at the direct map lookup, so either custom grains need
a supported pandas-frequency mapping or Prophet's schema must exclude them.
[api mismatch]
Severity Level: Major ⚠️
```mdx
- ❌ Forecasting fails for configured custom time grains.
- ⚠️ API validation advertises unsupported forecast values.
- ⚠️ Chart queries return validation errors instead of forecasts.
```
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ee8cebccc684498cb2de9971dea844a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ee8cebccc684498cb2de9971dea844a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
Prompt for AI Agent 🤖
```mdx
This is a comment left during a code review.
**Path:** superset/utils/pandas_postprocessing/utils.py
**Line:** 87:112
**Comment:**
*Api Mismatch: The mapping still contains only built-in `TimeGrain`
values, while `ChartDataProphetOptionsSchema` also accepts keys from
`TIME_GRAIN_ADDONS`. A configured custom grain will pass schema validation and
then fail in `prophet()` at the direct map lookup, so either custom grains need
a supported pandas-frequency mapping or Prophet's schema must exclude them.
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 fix
```
👍
| 👎
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
bito-code-review[bot] commented on PR #43205:
URL: https://github.com/apache/superset/pull/43205#issuecomment-5300579282
The flagged issue is correct. The `PROPHET_TIME_GRAIN_MAP` in
`superset/utils/pandas_postprocessing/utils.py` was missing mappings for
several time grains, which caused `prophet()` to fail when encountering them,
despite them being valid according to the schema. The provided PR diff already
includes a fix by adding these missing mappings to `PROPHET_TIME_GRAIN_MAP` and
adding unit tests to ensure all `TimeGrain` values are covered.
Since the fix is already implemented in the PR, no further action is
required for this specific comment.
**superset/utils/pandas_postprocessing/utils.py**
```
PROPHET_TIME_GRAIN_MAP: dict[str, str] = {
TimeGrain.SECOND: "s",
+TimeGrain.FIVE_SECONDS: "5s",
+TimeGrain.THIRTY_SECONDS: "30s",
TimeGrain.MINUTE: "min",
TimeGrain.FIVE_MINUTES: "5min",
TimeGrain.TEN_MINUTES: "10min",
TimeGrain.FIFTEEN_MINUTES: "15min",
TimeGrain.THIRTY_MINUTES: "30min",
+# An alternate ISO-8601 spelling of THIRTY_MINUTES that a number of
engine
+# specs expose instead; the two denote the same interval.
+TimeGrain.HALF_HOUR: "30min",
TimeGrain.HOUR: "h",
+TimeGrain.SIX_HOURS: "6h",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
codecov[bot] commented on PR #43205: URL: https://github.com/apache/superset/pull/43205#issuecomment-5300576187 ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43205?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 57.26%. Comparing base ([`21ae918`](https://app.codecov.io/gh/apache/superset/commit/21ae918656c808c43df7214a7979f85800f4fc43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b6eee23`](https://app.codecov.io/gh/apache/superset/commit/b6eee23f31565b6bea21e5bdd532fb1c73d35d17?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). > :exclamation: There is a different number of reports uploaded between BASE (21ae918) and HEAD (b6eee23). Click for more details. > > HEAD has 55 uploads less than BASE > >| Flag | BASE (21ae918) | HEAD (b6eee23) | >|--|--|--| >|python|44|2| >|presto|8|1| >|hive|7|1| > Additional details and impacted files ```diff @@Coverage Diff @@ ## master #43205 +/- ## == - Coverage 66.65% 57.26% -9.39% == Files2873 2873 Lines 163626 163626 Branches3776737767 == - Hits 10905993695 -15364 - Misses 5243869042 +16604 + Partials 2129 889-1240 ``` | [Flag](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [hive](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.17% <ø> (ø)` | | | [mysql](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [postgres](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [presto](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.11% <ø> (ø)` | | | [python](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.17% <ø> (-19.10%)` | :arrow_down: | | [sqlite](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [unit](https://app.codecov.io/gh/apache/superset/pull/43205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43205?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please
Re: [PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
bito-code-review[bot] commented on PR #43205: URL: https://github.com/apache/superset/pull/43205#issuecomment-5300560526 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
[PR] fix(chart): map the remaining time grains for Prophet forecasting [superset]
SEPURI-SAI-KRISHNA opened a new pull request, #43205:
URL: https://github.com/apache/superset/pull/43205
### SUMMARY
Enabling **Forecast** (Advanced Analytics → Predictive Analytics) fails with
`Unsupported time grain` for five time grains that Superset itself offers in
the Time Grain control.
`PROPHET_TIME_GRAIN_MAP` covered 16 of the 21 `TimeGrain` values. `prophet()`
rejects anything missing from it:
```python
if time_grain not in PROPHET_TIME_GRAIN_MAP:
raise InvalidPostProcessingError(_("Unsupported time grain:
%(time_grain)s", ...))
```
The five gaps, and how reachable each is — every one is exposed by real
engine
specs through their `_time_grain_expressions`:
| TimeGrain | ISO | engine specs offering it |
| --- | --- | --- |
| `FIVE_SECONDS` | `PT5S` | 40 (Postgres, CockroachDB, SQLite, DataFusion,
…) |
| `THIRTY_SECONDS` | `PT30S` | 42 (Postgres, CockroachDB, SQLite,
DataFusion, …) |
| `HALF_HOUR` | `PT0.5H` | 15 (SQLite, Presto, GSheets, Kusto, …) |
| `SIX_HOURS` | `PT6H` | 15 (SQLite, Presto, Druid, GSheets, …) |
| `QUARTER_YEAR` | `P0.25Y` | 10 (SQLite, Shillelagh, GSheets, Ocient, …) |
Three of them are also advertised as valid by the API contract:
`ChartDataProphetOptionsSchema.time_grain` validates with
`validate.OneOf(choices=get_time_grain_choices())`, and `PT5S`, `PT30S` and
`PT6H` are all in `builtin_time_grains`. So the OpenAPI spec publishes them
as
accepted values for the forecast operation, and the request then fails.
Two of the five are simply alternate ISO-8601 spellings of grains that
already
work — `PT0.5H` is `PT30M`, `P0.25Y` is `P3M` — so picking one spelling
forecasts and the other errors, for the same interval.
Reproducing on `master`:
```python
>>> prophet(df=prophet_df, time_grain="PT6H", periods=3,
confidence_interval=0.9)
InvalidPostProcessingError: Unsupported time grain: PT6H
```
This PR maps all five to their pandas frequency aliases (`5s`, `30s`,
`30min`,
`6h`, and the same quarter alias `QUARTER` already uses). No `TimeGrain`
value
is left unmapped.
Related but distinct: #42145 fixed `prophet()` being called with a *missing*
`time_grain`. This fixes grains that are present and valid but unmapped.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only fix.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/pandas_postprocessing/test_prophet.py
```
Three tests are added:
| test | asserts |
| --- | --- |
| `test_prophet_previously_unmapped_time_grains` | all five grains now
produce a forecast (parametrized; fails on `master`) |
| `test_prophet_every_time_grain_is_mapped` | no `TimeGrain` is missing from
the map, so this cannot regress |
| `test_prophet_alias_time_grains_match_their_canonical_form` |
`HALF_HOUR`/`QUARTER_YEAR` resolve identically to `THIRTY_MINUTES`/`QUARTER` |
Manually: on a Postgres or SQLite dataset, build a time-series chart, set
Time
Grain to **6 hour** (or **5 second** / **30 second**), then enable Forecast
under Advanced Analytics. On `master` the chart errors with "Unsupported time
grain"; with this change it forecasts.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### CHECKLIST
- [ ] CI checks pass
- [x] Tests added/updated
- [ ] Documentation updated
- [x] PR title follows conventions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
