Skip to content

fix(tsfm): allowlist the transformers and estimators recipes use - #639

Closed
ShuxinLin wants to merge 1 commit into
aafeedback_changes_tsfm_toolsfrom
fix/tsfm-allowlist-recipe-estimators
Closed

ShuxinLin wants to merge 1 commit into
aafeedback_changes_tsfm_toolsfrom
fix/tsfm-allowlist-recipe-estimators

Conversation

@ShuxinLin

Copy link
Copy Markdown
Collaborator

Description

Fixes the allowlist issue from the #633 review (comment on resolver.py:33). The allowlist has no sktime transformers and no regressors, so these always fail with ... is not an allowed estimator class:

  • the forecasting transforms block in run_recipe
  • tabular sktime_class transforms in run_tabular_recipe
  • the tsfm_regression tabular task

recipe_template advertises all three, and they worked before #633.

08e817b fixed this, but ebd792c reverted it, so the #633 head is back to the failing state. This PR covers only the allowlist. The other two review points (open-then-verify reads, confining training_args.output_dir) are not part of it.

Fix Details

substrate/resolver.py adds 33 exact paths to ALLOWED_CLASSES in three groups:

  • run_recipe transforms, plus the classes they nest: Detrender, Deseasonalizer, ConditionalDeseasonalizer, Differencer, LogTransformer, BoxCoxTransformer, ExponentTransformer, SqrtTransformer, Imputer, HampelFilter, TabularToSeriesAdaptor, PolynomialTrendForecaster, TrendForecaster, StandardScaler, MinMaxScaler, RobustScaler
  • run_tabular_recipe transforms: SummaryTransformer, Catch22, Rocket, MiniRocket, MiniRocketMultivariate
  • run_tabular_recipe estimators: RandomForestRegressor, GradientBoostingRegressor, ExtraTreesRegressor, LinearRegression, Ridge, Lasso, KNeighborsRegressor, ExtraTreesClassifier, GradientBoostingClassifier, LogisticRegression, KNeighborsClassifier, KMeans

None of these takes a file path or a pickle. Some take a callable (KNeighbors* metric/weights, ConditionalDeseasonalizer seasonality_test, KMeans init). A JSON recipe can only fill those with plain values or with instances _build makes from allowlisted classes, and none of those instances is callable. A new test checks that for every allowlisted class. FunctionTransformer stays out.

Impact on Benchmarking

Related Issues

Verification Steps

  1. uv run pytest src/servers/tsfm/tests -k "not integration": 149 passed (145 before plus 4 new). All the existing code-execution tests still pass.
  2. New tests in test_no_code_exec.py:
    • test_every_allowed_class_imports_and_is_not_callable: imports every path, which catches typos or sktime path drift, and checks that no instance is callable.
    • test_forecasting_transforms_still_run, test_tabular_regression_still_runs, test_tabular_sktime_transform_still_runs: one per broken route.
  3. Manually, each of the 33 added classes succeeded end to end through run_recipe / run_tabular_recipe (32/32 runs; the scalers and trend forecasters run nested via _target_).
  4. uv run pytest src/ -k "not integration": 769 passed. The 2 failures in src/observability/tests/test_file_exporter.py fail the same way on the Aafeedback changes tsfm tools #633 head without this change.

Checklist

  • I have added tests that prove my fix is effective.
  • My code follows the project's Ruff formatting and linting rules (no new findings on the changed files).
  • I have signed off my commits (DCO).

#633's allowlist has no sktime transformers and no regressors, so
the forecasting `transforms` block, tabular `sktime_class` transforms
and `tsfm_regression` always failed with "not an allowed estimator
class". Add the common ones that worked before #633.

Callable-typed params (KNeighbors `metric`, ConditionalDeseasonalizer
`seasonality_test`) can only receive instances of allowlisted classes,
and a new test checks that none of those instances is callable.

Signed-off-by: Shuxin Lin <linshuhsin@gmail.com>
@ShuxinLin ShuxinLin closed this Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant