-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add guards against loading pkl files #9048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds security guards to prevent unsafe loading of pickle files in DSPy programs. Pickle files can execute arbitrary code when loaded, so this change requires explicit permission via either the dangerously_allow_pickle=True parameter or the DSPY_ALLOW_PICKLE=1 environment variable before loading .pkl files or programs saved with save_program=True.
- Added
dangerously_allow_pickleparameter todspy.load()andBaseModule.load()methods - Updated all existing tests to use
dangerously_allow_pickle=Truewhen loading pickle files - Added comprehensive test coverage for the new security guards
- Updated documentation with security warnings about pickle file risks
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dspy/utils/saving.py | Added dangerously_allow_pickle parameter and permission check to the load() function |
| dspy/primitives/base_module.py | Added dangerously_allow_pickle parameter and permission check to the load() method; added warning in save method |
| tests/utils/test_saving.py | Updated existing tests to use dangerously_allow_pickle=True; added new tests for permission requirements and environment variable behavior |
| tests/primitives/test_base_module.py | Updated existing tests to use dangerously_allow_pickle=True; updated test expectations to account for pickle loading warning |
| tests/predict/test_predict.py | Updated test to use dangerously_allow_pickle=True when loading |
| docs/docs/tutorials/saving/index.md | Added security warnings about pickle file risks and updated code examples |
| docs/docs/tutorials/games/index.ipynb | Added security warning and updated code example with dangerously_allow_pickle=True |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chenmoneygithub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
… and suggest saving with module.save(x.json)
|
Removed environment variable, changed from |
chenmoneygithub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Adds a guard against loading pkl files in individual programs without an argument or environment variable set.