Skip to content

fix: propagate default target column warnings - #443

Closed
Saloni-0465 wants to merge 1 commit into
sktime:mainfrom
Saloni-0465:fix/propagate-default-target-warning-clean
Closed

Saloni-0465 wants to merge 1 commit into
sktime:mainfrom
Saloni-0465:fix/propagate-default-target-warning-clean

Conversation

@Saloni-0465

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

N/A

What does this implement/fix? Explain your changes.

This PR ensures that warnings generated during data conversion are surfaced in the top-level validation output.

Previously, if target_column was not provided, to_sktime_format would default to using the first column and store a warning inside the adapter metadata. However, this warning was not included in validation["warnings"], which is what downstream users (including agents) are expected to check.

With this change, any warnings generated during conversion are now propagated to the top-level validation report for both synchronous and asynchronous data loading. This makes the behavior more transparent and easier to debug.

Does your contribution introduce a new dependency? If yes, which one?

No.

What should a reviewer concentrate their feedback on?

  • Whether metadata-level conversion warnings should be merged into top-level validation
  • Whether the helper location in executor.py is appropriate
  • Test coverage for sync and async loading

Any other comments?

Added regression tests for sync and async load_data_source warning propagation.

Validation:

  • python -m compileall src/sktime_mcp/runtime/executor.py tests/test_data_validation_warnings.py
  • Manual sync and async checks confirm the default-target warning appears in validation["warnings"]

Note: local pytest exits with code -1 before output in this environment, so I verified behavior directly.

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated CODEOWNERS.
  • I've added unit tests and made sure they pass locally where possible.

For new estimators

  • Not applicable

@Shashankss1205

Copy link
Copy Markdown
Collaborator

Verified on current main that the default-target warning only lives in metadata.validation.warnings; with this branch it reaches the top-level validation.warnings for both sync and async loaders. Full suite green. Merging (the new test file will be picked up by the repo-wide ruff format in the lint fix).

@Shashankss1205

Copy link
Copy Markdown
Collaborator

Landed on main as 6a7262e (squash-merged locally because the org policy rejects the merge API for this token). Thanks!

Shashankss1205 added a commit that referenced this pull request Sep 6, 2026
Squash-merged from #443 by @Saloni-0465.

Co-authored-by: Saloni-0465 <Saloni-0465@users.noreply.github.com>
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.

2 participants