[AINode] Update forecast interface #16978
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the AINode forecast interface to support more flexible forecasting capabilities. It introduces optional history and future covariates to the thrift interface and unifies parameter naming across the codebase by renaming predict_length to output_length for consistency.
- Adds optional
historyCovsandfutureCovsfields to the thriftTForecastReqstruct - Renames
predict_lengthparameter tooutput_lengthacross all Python models and configuration files - Refactors Python forecast pipelines to accept list-based input structures with support for covariates
- Changes Java class member visibility from
privatetoprotectedto enable inheritance/extensibility
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| iotdb-protocol/thrift-ainode/src/main/thrift/ainode.thrift | Adds optional historyCovs and futureCovs fields to TForecastReq for covariate support |
| iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/function/tvf/ForecastTableFunction.java | Changes visibility modifiers from private to protected for extensibility |
| iotdb-core/ainode/iotdb/ainode/core/model/timer_xl/pipeline_timer.py | Updates pipeline to use output_length parameter and adds preprocessing for covariates |
| iotdb-core/ainode/iotdb/ainode/core/model/sundial/pipeline_sundial.py | Updates pipeline to use output_length parameter and adds preprocessing for covariates |
| iotdb-core/ainode/iotdb/ainode/core/model/sktime/pipeline_sktime.py | Refactors pipeline methods to work with list-based inputs and output_length parameter |
| iotdb-core/ainode/iotdb/ainode/core/model/sktime/stl_forecaster/config.json | Renames predict_length to output_length in configuration |
| iotdb-core/ainode/iotdb/ainode/core/model/sktime/naive_forecaster/config.json | Renames predict_length to output_length in configuration |
| iotdb-core/ainode/iotdb/ainode/core/model/sktime/exponential_smoothing/config.json | Renames predict_length to output_length in configuration |
| iotdb-core/ainode/iotdb/ainode/core/model/sktime/arima/config.json | Renames predict_length to output_length in configuration |
| iotdb-core/ainode/iotdb/ainode/core/model/sktime/modeling_sktime.py | Updates model generation methods to use output_length parameter |
| iotdb-core/ainode/iotdb/ainode/core/model/sktime/configuration_sktime.py | Updates configuration definitions to use output_length instead of predict_length |
| iotdb-core/ainode/iotdb/ainode/core/model/chronos2/pipeline_chronos2.py | Enhances preprocessing, adds detailed forecast documentation, and uses output_length |
| iotdb-core/ainode/iotdb/ainode/core/manager/inference_manager.py | Updates to work with new list-based input structure and output_length parameter |
| iotdb-core/ainode/iotdb/ainode/core/inference/pipeline/basic_pipeline.py | Refactors base pipeline classes to support dictionary-based inputs with covariates |
| iotdb-core/ainode/iotdb/ainode/core/inference/inference_request_pool.py | Adapts request processing to use new input structure and output_length parameter |
| integration-test/src/test/java/org/apache/iotdb/ainode/it/AINodeForecastIT.java | Changes test method visibility to static for reusability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if inputs[0].get("past_covariates", None) or inputs[0].get( | ||
| "future_covariates", None |
There was a problem hiding this comment.
The condition checks if past_covariates or future_covariates exist but uses or logic. This means if only past_covariates is provided (which should be supported), the warning will still be logged. Consider checking each covariate type separately if they have different support levels, or clarify if both are truly unsupported.
| if inputs[0].get("past_covariates", None) or inputs[0].get( | ||
| "future_covariates", None | ||
| ): | ||
| logger.warning( | ||
| f"[Inference] Past_covariates and future_covariates will be ignored, as they are not supported for model {model_id}." | ||
| ) |
There was a problem hiding this comment.
The condition checks if past_covariates or future_covariates exist but uses or logic. This means if only past_covariates is provided (which should be supported), the warning will still be logged. Consider checking each covariate type separately if they have different support levels, or clarify if both are truly unsupported.
| if inputs[0].get("past_covariates", None) or inputs[0].get( | |
| "future_covariates", None | |
| ): | |
| logger.warning( | |
| f"[Inference] Past_covariates and future_covariates will be ignored, as they are not supported for model {model_id}." | |
| ) | |
| has_past_covariates = inputs[0].get("past_covariates") is not None | |
| has_future_covariates = inputs[0].get("future_covariates") is not None | |
| if has_past_covariates or has_future_covariates: | |
| if has_past_covariates and has_future_covariates: | |
| logger.warning( | |
| f"[Inference] Past_covariates and future_covariates will be ignored, as they are not supported for model {model_id}." | |
| ) | |
| elif has_past_covariates: | |
| logger.warning( | |
| f"[Inference] Past_covariates will be ignored, as they are not supported for model {model_id}." | |
| ) | |
| else: | |
| logger.warning( | |
| f"[Inference] Future_covariates will be ignored, as they are not supported for model {model_id}." | |
| ) |
| if inputs[0].get("past_covariates", None) or inputs[0].get( | ||
| "future_covariates", None |
There was a problem hiding this comment.
The condition checks if past_covariates or future_covariates exist but uses or logic. This means if only past_covariates is provided (which should be supported), the warning will still be logged. Consider checking each covariate type separately if they have different support levels, or clarify if both are truly unsupported.
| super().postprocess(outputs_list, **infer_kwargs) | ||
| return outputs_list |
There was a problem hiding this comment.
The method calls super().postprocess() but doesn't use its return value. The result is stored in outputs_list which is then returned directly. Either remove the super() call if it's not needed, or use its return value instead of returning outputs_list.
| super().postprocess(outputs_list, **infer_kwargs) | |
| return outputs_list | |
| processed_outputs = super().postprocess(outputs_list, **infer_kwargs) | |
| return processed_outputs if processed_outputs is not None else outputs_list |
| ) | ||
| # If targets is 2-d, check if the second dimension is input_length | ||
| if targets.ndim == 2: | ||
| n_variates, input_length = targets.shape |
There was a problem hiding this comment.
The variable 'n_variates' is assigned on line 104 but never used. If it's not needed for any validation or processing, consider removing this assignment or using it for additional validation logic.
| n_variates, input_length = targets.shape | |
| _, input_length = targets.shape |
| else: | ||
| input_length = targets.shape[ | ||
| 0 | ||
| ] # for 1-d targets, shape should be (input_length,) |
There was a problem hiding this comment.
The comment states "for 1-d targets, shape should be (input_length,)" but doesn't reflect that for 1D targets, you're not validating anything about target_count. The comment might be misleading since it seems to imply a specific expected structure that isn't actually enforced.
| ] # for 1-d targets, shape should be (input_length,) | |
| ] # for 1-d targets, infer input_length from the first (and only) dimension |
2 participants
Description