Move AsyncMinHashLSH from experimental to datasketch.aio

[ekzhu]

Summary

Promotes AsyncMinHashLSH out of the datasketch.experimental namespace into a first-class datasketch.aio submodule, deprecates the old import paths, and runs the storage-backend integration tests directly on PRs. This is a re-submission of #301 from a new branch, with the description rewritten to accurately reflect the changes.

What does this PR do?

New datasketch.aio submodule

• Adds datasketch/aio/__init__.py, datasketch/aio/lsh.py, and moves datasketch/experimental/aio/storage.py to datasketch/aio/storage.py.
• Exposes AsyncMinHashLSH, AsyncMinHashLSHInsertionSession, and AsyncMinHashLSHDeleteSession from datasketch.aio.
• Re-exports AsyncMinHashLSH from the top-level datasketch package so from datasketch import AsyncMinHashLSH works. The import itself is always safe; instantiation still requires motor / redis.asyncio to be installed.

Deprecation of datasketch.experimental

• datasketch.experimental, datasketch.experimental.aio, and datasketch.experimental.aio.lsh now re-export from datasketch.aio and emit a DeprecationWarning on attribute access.
• Warnings are wired up via PEP 562 __getattr__ and cached back into globals() so the warning fires exactly once per process and intermediate package imports don't trigger redundant warnings.
• TYPE_CHECKING re-exports keep static analyzers happy without importing the real symbols eagerly.

Packaging

• Adds a new aio optional-dependency group in pyproject.toml (aiounittest, motor>3.6.0).
• Keeps experimental_aio as a duplicated alias for backwards compatibility (PEP 621 does not support referencing another optional-dependency group).
• Drops */experimental/* from the coverage omit list now that the module is no longer experimental-only.

Docs

• docs/documentation.rst and docs/lsh.rst updated to point at datasketch.aio.AsyncMinHashLSH.
• Removes the "currently experimental, interface may change" note from the async LSH docs.

CI

• test-cassandra.yml, test-mongo.yml, and test-redis.yml now trigger on push / pull_request against master directly instead of cascading off a completed Test workflow run, so integration tests actually gate PRs.

Tests

• test/aio/test_lsh.py updated to import from the new location and gains a test for the get_subset_counts async method.

Backwards compatibility

Old imports still work:

from datasketch.experimental import AsyncMinHashLSH            # DeprecationWarning
from datasketch.experimental.aio import AsyncMinHashLSH        # DeprecationWarning
from datasketch.experimental.aio.lsh import AsyncMinHashLSH    # DeprecationWarning

New preferred imports:

from datasketch.aio import AsyncMinHashLSH
from datasketch import AsyncMinHashLSH

Checklist

• Are unit tests passing?
• Documentation added/updated for all public APIs?
• Is this a breaking change? No — old imports keep working behind a DeprecationWarning.

https://claude.ai/code

[gemini-code-assist]

Code Review

This pull request transitions the asynchronous MinHash LSH implementation from an experimental status to a stable datasketch.aio module, including deprecation proxies for the old paths. The changes also introduce a getmany method for storage backends and update the project configuration to include an aio extra. Feedback focuses on resolving a potential type error regarding the storage basename, restoring explicit storage initialization to prevent runtime issues, and optimizing MongoDB's getmany implementation with asyncio.gather.

[ekzhu]

@dipeshbabu could you help review this PR?

[dipeshbabu]

This now hardcodes lrange(), but AsyncRedisSetStorage inherits getmany() too. Before this, the call went through _get_items(), so the set-
backed subclass used smembers() correctly. As written, unordered Redis storage will issue LRANGE against set keys and fail with WRONGTYPE.

[ekzhu]

Good catch — fixed in feead1d.

AsyncRedisListStorage.getmany now routes through _get_items, matching the existing itemcounts / _get_len pattern so AsyncRedisSetStorage's smembers override is used when the method is called on set-backed storage:

async def getmany(self, *keys):
    pipe = self._redis.pipeline()
    for key in keys:
        await self._get_items(pipe, self.redis_key(key))
    return await pipe.execute()

I also added a regression test (test_unordered_storage_getmany in test/aio/test_lsh.py) that calls getmany on an unordered hashtable storage, which would have hit WRONGTYPE against Redis sets under the old implementation.

On the CI suggestion — updated the three integration-test workflows (test-mongo.yml, test-redis.yml, test-cassandra.yml) to install the new aio extra instead of the deprecated experimental_aio alias. Both still resolve to the same dependency set, but using aio aligns CI with the new public API surface as you suggested.

---

Generated by Claude Code

[dipeshbabu]

There’s one blocking issue here in the Redis async storage path.

AsyncRedisListStorage.getmany() now calls lrange() directly. That works for the list-backed storage, but AsyncRedisSetStorage inherits the same method. Before this change, getmany() went through _get_items(), so the set-backed subclass used smembers() correctly. With the current version, unordered Redis storage will send LRANGE to set keys and fail with WRONGTYPE.

I think this should be fixed before merge, either by routing getmany() back through _get_items() or by overriding it in AsyncRedisSetStorage. Since datasketch.aio is now the public surface, it would be good to align CI/install coverage with the new aio extra too.

[dipeshbabu]

  The Redis `getmany()` issue I flagged is fixed in `feead1d`; routing through `_get_items()` restores the                    
  `AsyncRedisSetStorage`/`SMEMBERS` behavior, and the added regression test covers that path. The integration workflows are also now  
  installing the new `aio` extra, and the latest CI run is green.                                                                     
                                                                                                                                      
  I found one remaining blocker before promoting this out of `experimental`: `prepickle=True` is not applied consistently in the async
  public key APIs. `has_key()` and `_remove()` still use the caller's raw key, while `_insert()` stores the pickled key. This breaks  
  `has_key()` and `remove()` for non-bytes keys with `prepickle=True`.                                                                
                                                                                                                                      
  Please fix that and add regression coverage in `test/aio/test_lsh.py`, ideally under `TestAsyncMinHashLSHWithPrepickle` starting at 
  line 583. The tests should cover `has_key()`, `remove()`, and ideally `delete_session.remove()` with a string key.

[dipeshbabu]

This still misses prepickle=True for the public key APIs. _insert() pickles the key before storing it, and query() unpickles
returned candidates, but has_key() receives the caller's raw key here. With prepickle=True, this returns False for a key that
was just inserted:

async with AsyncMinHashLSH(storage_config=storage_config, prepickle=True) as lsh:                                                   
    await lsh.insert("a", m)                                                                                                        
    assert await lsh.has_key("a")  # currently False                                                                                
                                                                                                                                    
_remove() has the same issue on lines 291-300: it checks and fetches/removes using the raw key, so await lsh.remove("a") raises     
ValueError after inserting "a" with prepickle=True.                                                                                 
                                                                                                                                    
Please normalize/pickle keys consistently for has_key() and _remove(). Be careful not to double-pickle the key in _insert(), since  
_insert() currently calls has_key() after it has already pickled the key.

[ekzhu]

Verified the bug and fixed in 85907e7.

You're right on both counts — has_key() was forwarding the raw user key to storage, and _remove() was using the raw key for the existence check, the keys.get lookup, and the deletes. Under prepickle=True storage holds pickled bytes, so:

await lsh.insert("a", m)
await lsh.has_key("a")   # returned False
await lsh.remove("a")    # raised ValueError

I mirrored the sync MinHashLSH._remove pattern (datasketch/lsh.py:520-522): pickle inside _remove first, then call storage primitives directly so the existence check, lookup, and deletes all see the stored form.

To handle the double-pickling concern in _insert: has_key() now pickles internally for the public API, but _insert was already pickling the key before its has_key call. Rather than depend on subtle ordering, I switched _insert's duplicate-check call to self.keys.has_key(key) directly, since key is already in its stored (pickled) form at that point.

Added three regression tests under TestAsyncMinHashLSHWithPrepickle covering exactly the surface you called out: has_key, remove, and delete_session.remove with string keys (all parameterized over both mongo and redis storage).

---

Generated by Claude Code

[dipeshbabu]

@ekzhu, looks good now; i didn't see any blockers.