feat: Improving checksum generation robustness and usability - #18
Open
manasaV3 wants to merge 5 commits into
Open
feat: Improving checksum generation robustness and usability#18manasaV3 wants to merge 5 commits into
manasaV3 wants to merge 5 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
manasaV3
force-pushed
the
mvenkatakrishnan/checksum_refactor
branch
2 times, most recently
from
April 27, 2026 09:31
f9b19cf to
eb1d736
Compare
manasaV3
changed the title
manasaV3
force-pushed
the
mvenkatakrishnan/checksum_refactor
branch
from
May 7, 2026 02:31
a266ff1 to
c40f1ce
Compare
surygeng
reviewed
Jul 15, 2026
| Compute a checksum for a local path or S3 URI (s3:// or s3a://). | ||
| Delegates to compute_checksum_s3 or compute_checksum_localfs. | ||
| """ | ||
| if path.startswith(("s3://", "s3a://")): |
Collaborator
There was a problem hiding this comment.
Just out of curiosity, do we have s3a:// data location? It looks like the checksum supports only s3:// by checking the storage_platform.
Collaborator
Author
There was a problem hiding this comment.
We don't right now. It was more a of an edge case that was pre-emptively handled. :)
surygeng
approved these changes
Jul 15, 2026
Replace catalog_client/utils/checksums.py with a catalog_client/utils/checksum package split by concern: algorithm (hasher construction), hashing (streaming and Merkle combination), s3 (stored-checksum discovery), generate (asset and location entry points), and models. - Adds crc64 (ECMA-182) and crc64nvme alongside blake3, blake2b, and crc32 - Supports AssetType.folder via Merkle-root combination of per-file digests - Reuses checksums S3 already stores instead of downloading when possible - Moves the optional hashing dependencies into a "checksum" extra and imports them lazily, so importing catalog_client never requires them - Adds docs/checksum_guide.md and updates the USAGE.md checksum section Note: the alpha checksum API changes shape. generate_for_assets is now for_assets, and get_supported_algorithms is replaced by the Algorithm enum. Both are re-exported from catalog_client.utils alongside the manifest helpers.
manasaV3
force-pushed
the
mvenkatakrishnan/checksum_refactor
branch
from
August 8, 2026 05:07
12d55c8 to
4943680
Compare
The added `uv sync --group dev --extra checksum` step was undone by the following `uv run --group dev pytest`, which re-resolves the environment without `--extra checksum` and prunes blake3, crcmod and awscrt back out. Collapse both into a single `uv run --group dev --extra checksum pytest` so the extras are present for the tests that need them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guide described algorithm auto-detection as a two-tier order that preferred native S3 checksums over user metadata. The implementation is a single `max()` over `ALGORITHM_PRIORITY`, where blake3 (100) outranks the native crc32 (60) and crc64nvme (70) — so the documented order was inverted. Restate the real ranking, point at the source constant, and document the folder rule (strongest algorithm common to every child). Other corrections: - crc64 was listed as a native S3 checksum field; `_S3_NATIVE_RESPONSE_KEY` contains only crc32 and crc64nvme. - crc32 was credited to `binascii`; `_CRC32Hasher` uses `zlib`. - The local-filesystem example used `StoragePlatform.local`, which is not a member of the enum and raised AttributeError. Use `sf_hpc` and add the imports the snippet was missing. - The S3 `for_location` example printed None, because `for_location` defaults `compute_if_no_s3_checksum=False` while `for_assets` defaults True. Fix the example and document the asymmetry. - The caching example passed a literal `...`, omitted the required `asset_type`, and imported ChecksumResult from the internal models module rather than the package root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_hash_stream accumulated each 256MB chunk in an io.BytesIO and then called buf.getvalue(), which returns a second full copy — so peak memory scaled with CHUNK_SIZE and every byte was copied twice before being hashed. Keep a second live hasher for the current chunk and feed each 64KB read to both hashers directly. Peak memory drops from ~512MB to one read buffer regardless of CHUNK_SIZE. The flush condition is unchanged, so chunk boundaries land in exactly the same byte positions. Output is bit-identical: verified across 8 file sizes x 5 algorithms, comparing file_hash, merkle_root and the full chunk manifest (index, offset, size, hash) against the previous implementation, including the case where reads do not divide evenly into chunks. Cleanup carried in the same commit, since the source and test changes are interdependent: - Delete dead code: CRC_ALGORITHMS, CRYPTO_ALGORITHMS, and the _Hasher.raw() protocol method with its three implementations, none of which had call sites. - Move _raw_from_hex to algorithm.py as raw_from_hex, so per-algorithm digest widths are declared once beside the hashers rather than in two places. - Extract _directory_result() from the byte-identical block duplicated between _hash_local_dir and the S3 hash_tree closure. - Move the S3 key-tree builder from hashing.py to s3.py as _insert_key; it was S3-specific logic in the backend-agnostic module, and the only module-level function without an underscore prefix. - Import boto3 lazily in for_assets: importing catalog_client no longer pulls in boto3 and botocore (531 -> 311 modules). - Replace a string literal sitting mid-branch in new_hasher (a no-op, not a docstring) with a comment, and compare Algorithm members rather than bare strings. - Correct the module docstring, which listed boto3 as optional although it is a required dependency. Tests: - Replace three copy-pasted _setup_bucket methods with one s3 fixture that owns the mock_aws context, since a @mock_aws class decorator wraps only the test method and would leave fixture setup outside the mock. - Use pytest.warns and tmp_path in place of hand-rolled warnings.catch_warnings and NamedTemporaryFile/os.unlink boilerplate, preserving the strict single-warning assertions. - Delete one fully subsumed test, parametrize two pairs that differed only by an argument, drop never-passed helper parameters, and replace a positional call_args index with assert_called_once_with. - Add tests/utils/checksum/__init__.py to match every other test package and avoid test module basename collisions. - Record why the moto-backed suite and the mocked suite both exist: the mocked suite patches out the code the moto suite exercises. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A digest now depends only on content, never on where that content lives or how it was reached. A file hashes to the same value standalone as it does inside a folder, and a checksum read from S3 is comparable with one computed by downloading the object. The two roles a digest plays -- what a location reports, and what a child contributes to its parent -- were reading different fields (file_hash vs merkle_root), so the same folder produced different roots depending on whether its children had stored checksums. Both now read ChecksumResult.content_digest. Also in service of that property: - Route folder assets by asset_type instead of a trailing slash, and normalise prefixes so "s3://b/ds" neither fails with NoSuchKey nor sweeps in "s3://b/ds2/". - Ignore S3 multipart composite checksums (ChecksumType COMPOSITE, or a "-N" suffix): they cover part checksums, not object bytes, so their value depends on the uploader's part size. - Reject stored values that are not well-formed digests for their algorithm, per-algorithm, so one bad value cannot hide an object's good ones. - Fix crc64 to be the CRC-64/ECMA-182 it documents. crcmod's predefined "crc-64" is a different variant (check 0x46A5A9388A5BEFFE rather than 0x6C40DF5F0B497347). Caught by a new known-vector test. Behaviour and API changes: - Stop swallowing every HeadObject error. Only a missing object means "no stored checksum"; access, credential and throttling errors propagate instead of silently forcing a download or a skip. - Reuse cached folder children under compute_if_no_s3_checksum=False when the algorithm was auto-detected, matching the explicit-algorithm path. - default_algorithm() returns blake3 when installed and blake2b otherwise, so a base install without the checksum extra produces checksums rather than warning on every asset. - for_assets returns copies via model_copy() and no longer mutates caller assets; it is generic so DataAssetResponse survives the round trip. - Every skip reports through ChecksumWarning, so one warnings filter catches all of them. - Align compute_if_no_s3_checksum=True across for_location and for_assets. - Drop the generic for_assets / for_location / compute_checksum re-exports from catalog_client.utils; import them from catalog_client.utils.checksum. Removes catalog_client.utils.checksums with no shim. Shipped as a feature release, not a major bump: the checksum API is alpha and documented as subject to change. docs/checksum_guide.md carries a migration table. Adds 115 tests, including a reproducibility suite that pins the property above across four algorithms, and aligns the pre-commit mypy hook with the mypy pin in pyproject.toml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
|
|
||
| class _Hasher(Protocol): | ||
| def update(self, data: bytes) -> None: ... |
| class _Hasher(Protocol): | ||
| def update(self, data: bytes) -> None: ... | ||
|
|
||
| def hexdigest(self) -> str: ... |
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
2 participants
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.
Summary
Refactors checksum generation to be reliable and efficient when no algorithm is explicitly provided.
_hash_s3_prefix→_hash_s3_file, eliminating redundant S3 API calls during the compute phasecompute_if_no_s3_checksum=Falsenow correctly skips only S3 assets; non-S3 platforms (HPC, coreweave, local) always proceed to computations3a://support: URI parsing and platform detection now handle boths3://ands3a://schemes consistently