Make SDK models pickleable #8746
Conversation
WalkthroughThis update introduces the ability to pickle model instances within the SDK, allowing for their serialization and later reconstruction. A new test function has been added to verify this functionality specifically for the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Currently, they aren't (or rather, they can be pickled, but unpickling fails). This is due to a small quirk of how the model classes work, and is easily worked around. In addition, don't create a new Configuration object for each model. These objects are pretty beefy, and they increase the size of each pickle by a full kilobyte (and of course they increase memory usage even when pickle is not involved). AFAICS, these objects are only used when assigning values to file-type fields, and it's easy enough to rewrite the logic so that it still works when the model's `_configuration` field is None.
SpecLad
force-pushed
the
pickle-models
branch
from
c5ed34c
to
5020112
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
changelog.d/20241127_132256_roman_pickle_models.md (1)
1-4: Enhance the changelog entry with implementation details and impact.While the entry correctly documents the new feature, it would be more helpful to users if it included details about:
- The memory optimization achieved by eliminating per-model Configuration objects
- The behavior change regarding the
_configurationfield being allowed to be None- Any potential migration steps for users who might be relying on the previous behavior
Consider expanding the entry like this:
### Added -- [SDK] Model instances can now be pickled +- [SDK] Model instances can now be pickled. This change: + - Reduces memory usage by sharing Configuration objects between models + - Allows the model's `_configuration` field to be None when not needed + - Fixes unpickling issues in model classes (<https://github.com/cvat-ai/cvat/pull/8746>)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
cvat-sdk/gen/templates/openapi-generator/model_templates/method_from_openapi_data_composed.mustacheis excluded by!**/gen/**cvat-sdk/gen/templates/openapi-generator/model_templates/method_from_openapi_data_shared.mustacheis excluded by!**/gen/**cvat-sdk/gen/templates/openapi-generator/model_templates/method_from_openapi_data_simple.mustacheis excluded by!**/gen/**cvat-sdk/gen/templates/openapi-generator/model_templates/method_init_shared.mustacheis excluded by!**/gen/**cvat-sdk/gen/templates/openapi-generator/model_templates/method_init_simple.mustacheis excluded by!**/gen/**cvat-sdk/gen/templates/openapi-generator/model_utils.mustacheis excluded by!**/gen/**
📒 Files selected for processing (2)
changelog.d/20241127_132256_roman_pickle_models.md(1 hunks)tests/python/sdk/test_api_wrappers.py(2 hunks)
🔇 Additional comments (2)
tests/python/sdk/test_api_wrappers.py (2)
5-5: LGTM: Import placement is correct
The pickle import is appropriately placed with standard library imports.
118-124: Verify Configuration object memory optimization
The PR objectives mention optimizing memory usage by allowing _configuration to be None. Let's verify this behavior:
✅ Verification successful
Let me gather more specific information about the pickle-related implementation and configuration handling.
Based on the gathered information, I can now provide the final response:
Memory optimization for Configuration object is correctly implemented
The codebase shows that:
- The
_configurationfield is properly handled with a default value ofNonein model initialization methods - The pickle implementation in
model_utils.mustachecorrectly handles object serialization without requiring the Configuration object - The test case
test_models_are_pickleableverifies this behavior by successfully pickling and unpickling a model object
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Configuration object usage in model classes
# Look for Configuration-related code
rg -A 5 "class Configuration"
# Look for _configuration field usage
rg "_configuration\s*="
# Look for pickle-related methods
ast-grep --pattern 'def __getstate__($$$) { $$$ }'
Length of output: 2228
Script:
#!/bin/bash
# Check for pickle-related implementations and configuration handling in model classes
# Look for __getstate__ and __setstate__ methods in Python files
rg -U "def __[gs]etstate__" -A 5
# Look for specific configuration handling in model files
rg -U "_configuration" "tests/python/sdk" -A 2
# Check model base classes for configuration handling
ast-grep --pattern 'class $NAME($_):
$$$
_configuration = $$$
$$$'
Length of output: 1803
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8746 +/- ##
===========================================
- Coverage 74.05% 74.05% -0.01%
===========================================
Files 409 409
Lines 43783 43784 +1
Branches 3984 3984
===========================================
Hits 32425 32425
- Misses 11358 11359 +1
|
Motivation and context
Currently, they aren't (or rather, they can be pickled, but unpickling fails). This is due to a small quirk of how the model classes work, and is easily worked around.
In addition, don't create a new Configuration object for each model. These objects are pretty beefy, and they increase the size of each pickle by a full kilobyte (and of course they increase memory usage even when pickle is not involved). AFAICS, these objects are only used when assigning values to file-type fields, and it's easy enough to rewrite the logic so that it still works when the model's
_configurationfield is None.How has this been tested?
Unit tests.
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Tests
PatchedLabelRequestmodel.