fix: remove pysqlite3-binary auto-install via subprocess at import time - #7425
fix: remove pysqlite3-binary auto-install via subprocess at import time#7425rkfshakti wants to merge 3 commits into
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
eeshsaxena
left a comment
There was a problem hiding this comment.
Strongly agree with the intent. Shelling out to pip install as a side effect of import chromadb is a genuine anti-pattern - it does network I/O and mutates the environment at import time, then swaps sys.modules["sqlite3"] out from under everything else in the process. Replacing it with the same clear RuntimeError + docs link that every other platform already gets is the right call.
However, I think this breaks client startup as written. The PR deletes is_in_colab from chromadb/__init__.py, but it still has a live consumer:
# chromadb/telemetry/product/events.py
class ClientStartEvent(ProductTelemetryEvent):
def __init__(self) -> None:
super().__init__()
# Lazy import to avoid circular imports
from chromadb import is_in_colab
self.in_colab = is_in_colab()and ClientStartEvent is constructed unconditionally on the client-start path:
# chromadb/api/shared_system_client.py
def _submit_client_start_event(self) -> None:
telemetry_client = self._system.instance(ProductTelemetryClient)
telemetry_client.capture(ClientStartEvent())The event object is built before capture() gets to decide whether telemetry is even enabled, so ClientStartEvent() runs and hits ImportError: cannot import name 'is_in_colab' from 'chromadb'.
The nasty part is that the import is lazy (inside __init__), so this does not fail at import time - import chromadb still succeeds, and a smoke-import test in CI would stay green. It only blows up when a client is actually created, which is the very first thing a user does.
Two ways out, depending on what you want:
- Keep
is_in_colab, drop only the hotswap. Thegoogle.colabprobe is cheap and side-effect-free, and telemetry legitimately wants it. This PR's actual goal is removing thesubprocess/pip install/sys.modulessurgery, and that's fully achieved without deleting the predicate. - If you do want
is_in_colabgone from the public namespace,events.pyneeds to move the probe inline (or import it from an internal module) in the same PR.
Either way it'd be worth adding a test that constructs a client (not just imports the package), since that's the only thing that would have caught this.
Deferring to the maintainers.
|
Thanks @eeshsaxena for catching that! You're absolutely right — I've pushed a fix commit that restores
Could you take another look? |
|
Hi Team, just checking if there's anything else needed on this PR. Thanks! |
|
@rescrv @HammadB @codetheweb — this PR removes the subprocess/pip-install hotswap for pysqlite3-binary at import time (a known anti-pattern) while keeping the |
…or after removing pysqlite3 auto-install The PR removes is_in_colab() from chromadb/__init__.py, but ClientStartEvent in events.py still imported it via a lazy import. Inline the try/except google.colab check directly so the function can be safely deleted from __init__.py. Addresses eeshsaxena's review feedback on chroma-core#7425.
|
Thanks for the review @eeshsaxena — you were right that deleting |
|
Hi maintainers — just a friendly bump on this one. This removes the subprocess/pip-install hotswap for pysqlite3-binary at import time (a known anti-pattern). @eeshsaxena reviewed and agreed with the approach; the fix commit addresses the colab telemetry concern. Would appreciate a maintainer review when you get a chance. Thanks! |
|
Hi maintainers — just circling back on this one. I'm genuinely curious to hear your thoughts on the approach. The fix removes the subprocess/pip-install hotswap at import time while keeping the colab telemetry predicate intact. Happy to adjust if you'd prefer a different direction. Keen to contribute and learn from your feedback. Thanks! |
|
Friendly ping — this PR has been open for a couple of days with CI passing. Would appreciate a review when time allows. |
|
Hi maintainers — following up again on this fix for #7402. Removes the subprocess/pip-install hotswap for pysqlite3-binary and lets the system sqlite3 be used directly. CI is green. Would appreciate a review when time allows. Thanks! |
Running subprocess.check_call([sys.executable, '-m', 'pip', 'install', 'pysqlite3-binary']) at import time is fragile — it fails on non-standard architectures (ppc64, etc.) and is generally bad practice. Instead, raise a clear RuntimeError with instructions to install the dependency manually, consistent with the non-Colab error path. Closes chroma-core#7402
…tswap The is_in_colab() function is still used by chromadb/telemetry/product/events.py for telemetry. Only remove the subprocess.check_call pip install and the IN_COLAB constant, not the function itself.
…or after removing pysqlite3 auto-install The PR removes is_in_colab() from chromadb/__init__.py, but ClientStartEvent in events.py still imported it via a lazy import. Inline the try/except google.colab check directly so the function can be safely deleted from __init__.py. Addresses eeshsaxena's review feedback on chroma-core#7425.
7d54585 to
cfee4b6
Compare
Summary
Removes the that runs at import time when Chroma detects an old SQLite version in Google Colab.
Problem
Running
pip installvia subprocess at import time is fragile:Fix
subprocess.check_callauto-install blockis_in_colab()helper andIN_COLABconstantRuntimeErroras the non-Colab path, directing users to the troubleshooting guideCloses #7402