Skip to content

Conversation

@wesselvannierop
Copy link
Collaborator

@wesselvannierop wesselvannierop commented Jan 16, 2026

See test test_config_pickle, which dumps and loads a Config with pickle. Useful for our caching mechanism which uses pickle.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced Config object stability with defensive checks that guard against missing internal state attributes, preventing crashes when accessing configuration properties in certain edge cases.
  • Tests

    • Added test coverage to validate that Config instances can be properly serialized and deserialized while maintaining complete data integrity and object equivalence.

✏️ Tip: You can customize this high-level summary in your review settings.

@wesselvannierop wesselvannierop requested a review from a team January 16, 2026 15:33
@wesselvannierop wesselvannierop self-assigned this Jan 16, 2026
@wesselvannierop wesselvannierop requested review from OisinNolan and removed request for a team January 16, 2026 15:33
@wesselvannierop wesselvannierop added the bug Something isn't working label Jan 16, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

Walkthrough

This PR adds pickle serialization testing for Config instances and introduces defensive attribute existence checks to prevent AttributeError when accessing the internal __accessed__ tracking mechanism.

Changes

Cohort / File(s) Change Summary
Config Pickle Serialization
tests/test_configs.py
New test test_config_pickle validates that Config instances can be pickled and unpickled without data loss, using pickle.dumps/pickle.loads and equality assertion.
Config Attribute Guards
zea/config.py
Added hasattr(self, "_Config__accessed") checks in _mark_accessed and _mark_unaccessed to prevent AttributeError when the internal tracking attribute is uninitialized.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Configs can be pickled' directly and clearly summarizes the main objective of the PR: adding pickling support for Config objects.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
tests/test_configs.py (1)

254-267: Consider extending test coverage for pickle round-trip.

The test verifies value equality but doesn't confirm that:

  1. Nested objects are restored as Config instances (not plain dicts)
  2. Internal state like __frozen__ is preserved after unpickling

This could mask issues where pickling silently degrades Config functionality.

💡 Suggested additional assertions
     # Check if the config is the same
     config_check_equal_recursive(config, unpickled)
+
+    # Verify nested structures are still Config instances
+    assert isinstance(unpickled, Config)
+    assert isinstance(unpickled.nested_dictionary, Config)
+    assert isinstance(unpickled.nested_dictionary.doubly_nested_dictionary, Config)

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8613512 and 136987a.

📒 Files selected for processing (2)
  • tests/test_configs.py
  • zea/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_configs.py (1)
zea/config.py (1)
  • Config (62-494)
⏰ 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). (1)
  • GitHub Check: image
🔇 Additional comments (1)
zea/config.py (1)

281-289: Guards correctly prevent AttributeError during unpickling.

The hasattr(self, "_Config__accessed") checks are necessary because during dict unpickling, __setitem__ or __setattr__ may be called before instance attributes are restored. These guards prevent AttributeError in that window.

Note: The existing pickle test (test_configs.py:254) only verifies dict content preservation, not the metadata state (__accessed__, __parent__). However, since these are instance attributes set via super().__setattr__(), they are preserved by Python's default dict pickling. No explicit __getstate__/__setstate__ is required for basic functionality.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
zea/config.py 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants