Rotbaum: fall back to legacy predictor.pkl in TreePredictor.deserialize - #3294
Open
shaun0927 wants to merge 2 commits into
Open
Rotbaum: fall back to legacy predictor.pkl in TreePredictor.deserialize#3294shaun0927 wants to merge 2 commits into
shaun0927 wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: follow-up to #3176
Description of changes:
#3176 migrated
TreePredictorpersistence from pickle (predictor.pkl) to JSON (model_list.json).TreePredictor.deserializewas rewritten to hardcode the new path:Every Rotbaum predictor that was serialized with a v0.15.x release (which wrote
predictor.pkland nothing else) now fails withFileNotFoundError: .../model_list.jsonwhen loaded on v0.16+.Reproducer
Design choice: opt-in legacy load
Reintroducing pickle as an automatic fallback would resurrect the very reason #3176 migrated away from it —
pickle.loadexecutes arbitrary code on load. The first revision of this PR took that simpler path; I've changed it to require explicit opt-in so we don't regress on that front.TreePredictor.deserializenow:model_list.jsonif present (the post-Rotbaum: turn to json-based serialization #3176 format);predictor.pklis present AND the caller passedallow_legacy_pickle=True, loads it and emits aDeprecationWarningtelling the user to re-save so they migrate on their own schedule;predictor.pklis present but the flag is not set, raisesFileNotFoundErrorwith a one-line message that points users at the opt-in flag;FileNotFoundErrorwhen neither file is present.Pickle is only imported lazily inside the fallback branch, so the JSON happy path keeps the module's current zero-pickle import surface. The stale docstring that still said "loads the trained model list by reading the pickle file" is updated to describe the new behaviour.
Trade-offs considered
predictor.pklinto a path the user callsdeserializeon (shared model hub, S3 bucket with broad write perms) gets RCE via pickle. The opt-in flag keeps that surface closed by default.TreePredictor.allow_legacy_pickle = True): rejected. A per-call kwarg scopes the trust decision to the single load.Verification
Dynamic matrix with
super().deserializemocked out (that's the parent path, unchanged by this PR):By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup