Skip to content

[issue-9472] [SDK] fix: keep content hashes still shared after a dataset item is deleted - #9473

Open
BlueX888 wants to merge 1 commit into
comet-ml:mainfrom
BlueX888:BlueX888/issue-9472-keep-shared-content-hashes-on-delete
Open

BlueX888 wants to merge 1 commit into
comet-ml:mainfrom
BlueX888:BlueX888/issue-9472-keep-shared-content-hashes-on-delete

Conversation

@BlueX888

@BlueX888 BlueX888 commented Nov 5, 2026

Copy link
Copy Markdown

Details

Dataset.delete() dropped a content hash as soon as one of the ids carrying it was deleted. Content hashes are shared by items with identical content (for example after a deduplication=False insert), so deleting one of several same-content items made the next deduplicated insert upload a duplicate. Delete now rebuilds _hashes from the surviving _id_to_hash entries; a partially failed delete also marks the hash cache for re-sync, matching the insert failure path.

Change checklist

  • User facing
  • Documentation update

Issues

AI-WATERMARK

AI-WATERMARK: yes

  • Tools: Claude Code
  • Model(s): Claude Fable 5
  • Scope: implementation, unit tests, local verification
  • Human verification: the account owner authorized this autonomous contribution; the diff and test results were reviewed by an independent agent. No manual backend/UI session was run.

Testing

  • Base red on cc65247: the new dedup test fails with assert 2 == 1 (the re-insert after deleting one shared id is uploaded); the partial-failure count test's new hashes_synced assertion also fails.
  • Candidate: pytest tests/unit/api_objects/dataset/ -q → 440 passed.
  • uvx [email protected] check / format --check on the changed files → clean.
  • uvx [email protected] --config-file sdks/python/pyproject.toml --disable-error-code misc on dataset.py → success.
  • Tests use the existing UploadCapture / _StoringCapture fakes; no live backend or network.

Documentation

No user-facing documentation; this is an internal dedup-cache invariant.

@BlueX888
BlueX888 requested a review from a team as a code owner November 5, 2026 19:41
@github-actions github-actions Bot added python Pull requests that update Python code tests Including test files, or tests related like configuration. Python SDK 🟢 size/S labels Nov 5, 2026
@baz-reviewer

baz-reviewer Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Review this PR on Baz

Merger   ✅ Ready to Merge

The change correctly preserves shared content hashes and invalidates the cache after partial deletion. The only open discussions are low-severity test-strengthening suggestions, which do not block approval under the organization guidelines.


Comment on lines +815 to +824
# A deduplicated insert syncs the cache from the backend and skips the content.
dataset.insert([{"input": {"key": "same"}}])
assert capture.request_count == 1

dataset.delete([first["id"]])
dataset.insert([{"input": {"key": "same"}}])

assert capture.request_count == 1, (
"The second item still stores the content, so the re-insert must be deduplicated"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Deduplication test omits uploaded record verification

The deduplication test can pass with the wrong payload because it checks only the request count, not which records are uploaded or whether the existing duplicate is omitted. Assert the captured records for the initial uploads, verify reinserting the duplicate uploads nothing, and confirm a genuinely new item is uploaded.

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/api_objects/dataset/test_dataset_client.py` around lines
815-824, strengthen
`test_delete__content_hash_shared_by_a_surviving_item__reinsert_is_deduplicated` instead
of relying only on `capture.request_count`. Assert that the initial upload contains both
expected records, that reinserting the shared content produces no uploaded record, and
that a genuinely new item is uploaded, verifying the captured payloads and not just the
number of requests.

Comment on lines +313 to +315
assert dataset.__internal_api__hashes_synced__ is False, (
"A failed delete may have applied a batch, so the hash cache must re-sync"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Failure test checks private cache state only

The test can pass even when the next deduplicated insert skips records or uploads the wrong ones because it checks the private __internal_api__hashes_synced__ flag instead of observable behavior. Exercise that insert against the mocked backend and assert the uploaded records, including recovery from the partial deletion.

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/api_objects/dataset/test_dataset_items_count.py` around lines
313-315, update
`test_delete__fails_after_earlier_batches_were_deleted__still_invalidates_...` so it
does not assert the private `__internal_api__hashes_synced__` flag as the primary
outcome. After the failed partial delete, exercise a deduplicated insert through the
mocked backend and assert the uploaded records, proving that the cache/hash state
recovers and neither skips nor reuploads the wrong items.

This branch has not been deployed

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

Labels

baz: ready to merge Python SDK python Pull requests that update Python code 🟢 size/S tests Including test files, or tests related like configuration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Dataset.delete() drops a content hash still shared by another item, so a later insert duplicates content

1 participant