Pickle warning for itertools #6129
Conversation
WalkthroughAdds a pickle deprecation helper and emits DeprecationWarning from many itertools Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Iter as itertools::__reduce__
participant Dep as pickle_deprecation(vm)
participant Warn as warnings.warn
Caller->>Iter: call __reduce__(..., vm)
activate Iter
Iter->>Dep: pickle_deprecation(vm)
activate Dep
Dep->>Warn: emit DeprecationWarning ("removed in Python 3.14")
deactivate Dep
Iter-->>Caller: return original reduction tuple
deactivate Iter
note right of Iter: Applied across multiple itertools classes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vm/src/stdlib/itertools.rs (1)
1142-1181: Convert iterator structs to Python objects before tuple construction
In vm/src/stdlib/itertools.rs, thePyItertoolsChainandPyItertoolsAccumulateinstances must be converted via.into_pyobject(vm)before passing tovm.new_tuple, as they don’t implementToPyObject/IntoPyTupledirectly and will fail to compile otherwise.--- a/vm/src/stdlib/itertools.rs @@ 1158 - let chain = PyItertoolsChain { + let chain = PyItertoolsChain { source: PyRwLock::new(Some(chain_args.to_pyobject(vm).get_iter(vm).unwrap())), active: PyRwLock::new(None), - }; + }.into_pyobject(vm); @@ 1163 - let acc = Self { + let acc = Self { iterable: PyIter::new(chain), bin_op, initial: None, acc_value: PyRwLock::new(None), - }; + }.into_pyobject(vm);
🧹 Nitpick comments (2)
vm/src/stdlib/itertools.rs (2)
32-39: Fix deprecation message grammar and consider higher stacklevel.
- "Itertool" → "itertools"; "in a Python 3.14" → "in Python 3.14".
- Use stacklevel=2 (or 3) so the warning points closer to the user site that triggered pickling.
Apply:
fn pickle_deprecation(vm: &VirtualMachine) -> PyResult<()> { warnings::warn( vm.ctx.exceptions.deprecation_warning, - "Itertool pickle/copy/deepcopy support will be removed in a Python 3.14.".to_owned(), - 1, + "itertools pickle/copy/deepcopy support is deprecated and will be removed in Python 3.14.".to_owned(), + 2, vm, ) }Also please confirm the exact deprecation version text matches CPython’s plan for 3.14.
964-975: Confirm whether reading state via AtomicCell::take() is intentional.
take()resetscurandnextto0as a side-effect during reduction. If pickling should be read-only, considerload()instead and pass state explicitly.Example adjustment if needed:
- let cur = zelf.cur.take(); - let next = zelf.next.take(); + let cur = zelf.cur.load(); + let next = zelf.next.load();
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Lib/test/test_itertools.pyis excluded by!Lib/**
📒 Files selected for processing (1)
vm/src/stdlib/itertools.rs(16 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
vm/src/stdlib/itertools.rs
🧬 Code graph analysis (1)
vm/src/stdlib/itertools.rs (3)
vm/src/stdlib/warnings.rs (2)
warn(5-18)warn(39-48)vm/src/warn.rs (1)
warn(76-87)vm/src/builtins/function.rs (1)
__reduce__(788-796)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (13)
vm/src/stdlib/itertools.rs (13)
22-22: Import of warnings module looks fine.Consistent with using the stdlib warnings shim for emitting Python-level warnings.
84-99: Deprecation call placement is correct.Calling the helper before constructing the reduction tuple and using
?matches the method’sPyResultsignature.
217-223: Consistent handling for non-PyResultreturn.Using
let _ = pickle_deprecation(vm);is appropriate here since the method doesn’t returnPyResult.
288-291: LGTM for count.reduce.Deprecation hooked without altering existing reduction payload.
420-427: LGTM for repeat.reduce.Helper used with
?as the method returnsPyResult.
490-499: LGTM for starmap.reduce.Consistent pattern; no change to reduction data.
559-569: LGTM for takewhile.reduce.Deprecation integrated; state value unchanged.
647-657: LGTM for dropwhile.reduce.Deprecation integrated; state value unchanged.
1061-1070: LGTM for filterfalse.reduce.Pattern matches other reducers; no behavior change beyond the warning.
1407-1431: LGTM for product.reduce.Deprecation added; payload unchanged.
1517-1543: LGTM for combinations.reduce.Warning insertion is correct; state handling unaffected.
1759-1765: LGTM for permutations.reduce.Consistent with other reducers; no additional state serialized.
1873-1885: LGTM for zip_longest.reduce.Helper called with
?; reduction tuple intact.
| fn pickle_deprecation(vm: &VirtualMachine) -> PyResult<()> { | ||
| warnings::warn( | ||
| vm.ctx.exceptions.deprecation_warning, | ||
| "Itertool pickle/copy/deepcopy support will be removed in a Python 3.14.".to_owned(), |
There was a problem hiding this comment.
| "Itertool pickle/copy/deepcopy support will be removed in a Python 3.14.".to_owned(), | |
| "Itertools pickle/copy/deepcopy support will be removed in a Python 3.14.".to_owned(), |
Otherwise adding itertool to cspell dictionary will fix the CI failure
Summary by CodeRabbit