fix: materialize generators in sanitize_for_json - #504
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes JSON serialization of splitter outputs returned via Executor.call_method by ensuring iterator/generator results (and pd.Index) are materialized and recursively sanitized, preventing generator str() leakage (issue #490).
Changes:
- Materialize iterators/generators (and
pd.Index) insidesanitize_for_json. - Ensure
call_methodalways runssanitize_for_jsonon returned values, includingto_dictpaths, and avoid mutating live DataFrames when flattening MultiIndex columns. - Add tests covering iterator sanitization and splitter
split*methods returning fold lists instead of generator strings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_sanitize.py | Adds unit tests asserting generators/iterators and pd.Index are JSON-sanitized to lists. |
| tests/test_call_method.py | Adds regression tests for splitter split/split_loc/split_series returning fold lists via call_method. |
| src/sktime_mcp/server.py | Extends sanitize_for_json to handle iterators and pd.Index (but currently introduces an isinstance runtime error). |
| src/sktime_mcp/runtime/executor.py | Always sanitizes call_method results and avoids mutating MultiIndex DataFrames (but currently introduces an isinstance runtime error). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(obj, list | tuple): | ||
| return [sanitize_for_json(item) for item in obj] |
There was a problem hiding this comment.
checked this. we require python >=3.10, and isinstance accepts X | Y unions from 3.10 on. reprod fine on 3.10/3.11/3.12. sanitize and the split path still work. not a TypeError on our supported versions.
| if isinstance(obj, str | int | float | bool | type(None)): | ||
| return obj |
There was a problem hiding this comment.
same as prev.
| ) -> str | None: | ||
| """Return a stable frequency label for metadata without assuming datetime-only indexes.""" | ||
| if isinstance(index, (pd.DatetimeIndex, pd.PeriodIndex)): | ||
| if isinstance(index, pd.DatetimeIndex | pd.PeriodIndex): |
There was a problem hiding this comment.
same as prev.
| if freq is None and isinstance(y.index, pd.DatetimeIndex | pd.PeriodIndex): | ||
| # Try to infer |
There was a problem hiding this comment.
same as prev.
dc5b245 to
b3d79dd
Compare
|
The generator-materialisation half landed on main in 685195e (#520), so that part is now redundant, but the rest is still valuable: on current main split_loc folds come back as PeriodIndex([...]) repr strings, which your pd.Index branch fixes, and the .copy() before the MultiIndex rename plus always sanitising the to_dict path are good catches. Could you rebase onto main, drop the duplicated generator handling and the two split tests already covered by tests/test_call_method_generators.py, keep the pd.Index/copy/always-sanitize changes with their tests, and leave the isinstance style rewrites out? Then this is a quick merge. |
Generator materialization already landed in sktime#520. split_loc still leaked PeriodIndex reprs, to_dict skipped sanitize, and MultiIndex flatten mutated the live frame. Fixes sktime#490
b3d79dd to
c81852b
Compare
Reference Issues/PRs
Fixes #490.
What does this implement/fix? Explain your changes.
call_methodon splitters (e.g.SlidingWindowSplitter.splitwithy_dataset=airline) returnedsuccess: truebutresult: "<generator object ...>"because generators fell throughsanitize_for_jsontostr().Materialize generators/iterators (and
pd.Indexforsplit_loc) so fold data is JSON. Always runsanitize_for_jsononcall_methodresults, including theto_dictpath. Copy before MultiIndex column rename so we don't mutate live objects.Does your contribution introduce a new dependency? If yes, which one?
No.
What should a reviewer concentrate their feedback on?
sanitize_for_json(vs only listing incall_method)pd.Indexhandling forsplit_locAny other comments?
Verified with a real MCP stdio client: instantiate →
call_methodsplit → fold lists, not generator strings.make checkpasses locally.PR checklist
For all contributions
make check).docs/source/. (n/a — no tool surface change)examples/. (n/a)