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

Activity

1. github-actions commented on Nov 5, 2026

   

   github-actionsBot

   on Nov 5, 2026 – with GitHub Actions

   Contributor

   More actions

   Hi, I'm Scout 🦉, the comet-ml/opik repository agent. Let me look into this.

   Solution / Workaround

   No existing workaround found in the issue tracker. The reporter has already identified the root cause precisely and provided a concrete fix. The closest related issue is #9376 (now closed), which addressed the insert-side counterpart: a failed insert() leaving stale hashes in the cache.

   Code Investigation

   The bug lives in sdks/python/src/opik/api_objects/dataset/dataset.py, in the delete() method's loop that manages the in-memory hash cache.

   The core problem is that _hashes is a set of hash values while _id_to_hash is a dict mapping item id → hash. The hash-to-id relationship is not injective: two items can share the same hash (this is intentional and expected — deduplication=False explicitly allows it). The current delete loop does something like:

   for item_id in batch:
       hash_val = self._id_to_hash.pop(item_id, None)
       if hash_val:
           self._hashes.discard(hash_val)   # ← BUG: evicts even if another id still maps to it

   This incorrectly removes hash_val from _hashes as soon as any id that maps to it is deleted — even when other ids in _id_to_hash still reference the same hash. The next deduplicated insert() call then no longer finds the hash in _hashes, considers the content new, and uploads a duplicate.

   The second issue is on the failure path: _hashes_synced is left as True even when a batch fails mid-loop, leaving the cache in an inconsistent state (insert-side analogue was fixed in #9376/#9393).

   Test coverage: sdks/python/tests/unit/api_objects/dataset/test_dataset_client.py has good coverage of insert deduplication scenarios but no test exists for the delete-then-insert scenario described here. The upload_capture.py fake infrastructure is well-suited to add such a test without a live backend.

   Next Steps

   The reporter's suggested fix is correct and minimal. Apply it to the delete batch loop in dataset.py:

   completed = False
   try:
       for batch in batches:
           self._delete_batch_with_retry(batch, batch_group_id=batch_group_id)
           for item_id in batch:
               self._id_to_hash.pop(item_id, None)
       completed = True
   finally:
       self._dataset_items_count = None
       if completed:
           # Rebuild _hashes from surviving ids — a hash is only gone when
           # no id still references it.
           self._hashes = set(self._id_to_hash.values())
       else:
           # A batch may have been applied by the backend even if it raised;
           # force a re-sync on the next deduplicated insert.
           self._hashes_synced = False

   The key insight: rebuilding _hashes = set(self._id_to_hash.values()) after all pops is correct because set() deduplicates, so a hash shared by multiple surviving ids will appear exactly once — and a hash only disappears from _hashes when every id that referenced it has been deleted.

   A test should be added to test_dataset_client.py using the existing UploadCapture/make_dataset helpers, verifying:

   1. After insert([item, item], deduplication=False) + delete of one id, a subsequent deduplicated insert of the same content is skipped (no extra upload request).
   2. After a failed mid-delete, _hashes_synced is False.

   Feel free to open a pull request with this fix — the change is straightforward!

   Was this helpful? React to this comment with 👍 or 👎 to rate my response.

   ---

   View Scout trace in Opik
2. surlymochan commented on Nov 6, 2026

   

   surlymochan

   on Nov 6, 2026

   Contributor

   More actions

   Confirmed by reading the code (local checkout at 68c0725, 2026-11-05):

   • sdks/python/src/opik/api_objects/dataset/dataset.py:1264-1267 in delete(): for each deleted id it does self._hashes.discard(hash) unconditionally — no check whether another surviving id in _id_to_hash still maps to that hash. So deleting one of two same-content items drops the shared hash and the next deduplicated insert re-uploads it, exactly as reported.
   • Also confirmed the second part: the finally block only clears _dataset_items_count and never touches _hashes_synced, unlike the insert path which sets _hashes_synced = False on failure (lines ~959, ~1030) — so a mid-batch delete failure leaves the cache marked synced.

   The suggested fix (rebuild _hashes from the surviving _id_to_hash values after successful batches, and mark unsynced on failure) lines up with the code as written.