Skip to content

Conversation

@richardkiss
Copy link
Contributor

No description provided.

@richardkiss richardkiss requested review from arvidn and Copilot March 21, 2025 22:00
Copy link

Copilot AI left a 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 rebases long-ago written serialization code to support back‐reference functionality. Key changes include the introduction of an ObjectCache for calculating tree hashes and serialized lengths, refactoring of ReadCacheLookup and serialization routines to handle back-references, and the addition of extensive tests across serialization, object caching, and S-expression parsing.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
clvm/object_cache.py Adds ObjectCache class for caching tree hash/length.
clvm/read_cache_lookup.py Implements back-reference lookup functionality.
clvm/serialize.py Refactors serialization logic to support backrefs.
tests/read_cache_lookup_test.py Adds tests covering various lookup and cache behaviors.
tests/object_cache_test.py Introduces tests verifying caching functionality.
tests/serialize_test.py Updates tests to validate back-reference serialization.
setup.py Minor reformatting of package declaration.
clvm/SExp.py, clvm/operators.py, clvm/as_python.py, clvm/more_ops.py, clvm/run_program.py, clvm/CLVMObject.py Minor adjustments and formatting improvements.

@coveralls-official
Copy link

coveralls-official bot commented Mar 22, 2025

Pull Request Test Coverage Report for Build 14188466227

Details

  • 234 of 244 (95.9%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 94.128%

Changes Missing Coverage Covered Lines Changed/Added Lines %
clvm/object_cache.py 58 60 96.67%
clvm/serialize.py 92 100 92.0%
Totals Coverage Status
Change from base Build 14179366495: 0.2%
Covered Lines: 1203
Relevant Lines: 1262

💛 - Coveralls

@richardkiss richardkiss requested a review from Copilot March 22, 2025 20:50
Copy link

Copilot AI left a 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 rebases and merges long-ago serialization-with-backrefs code along with several test updates and minor formatting adjustments. Key changes include the introduction of back-reference handling in the serialization/deserialization routines, enhanced read cache lookup operations, and updates to test coverage and error message formatting across modules.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file
File Description
clvm/object_cache.py Introduces caching logic for CLVM objects without major logic changes.
clvm/read_cache_lookup.py Implements back-reference lookups and updates the pop and push operations.
tests/read_cache_lookup_test.py Adds extensive tests for read cache lookup functionality.
tests/object_cache_test.py Updates tests for the object cache functionality.
clvm/serialize.py Adds support for back-references in serialization along with new helper functions.
tests/serialize_test.py Updates tests to verify serialization with and without back-references.
tests/operators_test.py Minor formatting and consistency updates for operator tests.
clvm/as_python.py Small adjustments to type annotations.
tests/cmds_test.py Adjusts entry point import formatting for tool invocation tests.
tests/to_sexp_test.py Updates formatting and assertions for SExp conversion tests.
clvm/CLVMObject.py Updates error message formatting for tuple size validation.
clvm/run_program.py Minor formatting changes.
clvm/more_ops.py Improves error handling formatting for various eval functions.
Comments suppressed due to low confidence (2)

clvm/serialize.py:69

  • [nitpick] Consider defining enumerated constants or more descriptive names for the operation codes 'P' and 'C' used in the read_op_stack to improve readability and maintainability.
read_op_stack = ["P"]

clvm/CLVMObject.py:46

  • [nitpick] Consider using an f-string for the error message for better clarity and consistency, for example: f"tuples must be of size 2, cannot create CLVMObject from: {narrowed_v}".
raise ValueError("tuples must be of size 2, cannot create CLVMObject from: %s" % str(narrowed_v))

@altendky altendky closed this Mar 24, 2025
@altendky altendky reopened this Mar 24, 2025
@richardkiss richardkiss force-pushed the ser_br branch 7 times, most recently from cf74b55 to bdb689b Compare April 1, 2025 06:05
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider this more of a reference-implementation than a practical one.
It could probably do with some round-trip tests, including trees where nodes are reused.

@richardkiss
Copy link
Contributor Author

I consider this more of a reference-implementation than a practical one. It could probably do with some round-trip tests, including trees where nodes are reused.

Agreed. It's a good baseline for some improvements I'm trying, in particular one that I believe turns an $n^2$ factor into an ${n} log {n}$ one.

@richardkiss richardkiss merged commit ec8bc6e into main Apr 2, 2025
21 checks passed
@richardkiss richardkiss deleted the ser_br branch April 2, 2025 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants