nomic-ai / nomic

Interact, analyze and structure massive text, image, embedding, audio and video datasets
https://atlas.nomic.ai
1.27k stars 168 forks source link

embed: fix regression in GPT4All detection #302

Closed cebtenzzre closed 4 months ago

cebtenzzre commented 4 months ago

Since #292 (released in 3.0.25, as 3.0.24 doesn't exist), the nomic client always assumes GPT4All is installed and fails to display The 'gpt4all' package is required for local inference., instead erroring later when it fails to find CancellationError. This is because elif embed4all_installed is None: does not work, because embed4all_installed is always True or False.

This PR fixes that by restoring the original pattern, and adding an additional TYPE_CHECKING guard.

I also fixed a few recent additions of if isinstance(...) that made no sense - the pattern is supposed to be assert isinstance(...), otherwise you are potentially introducing a bug. They were made unnecessary by adding a proper set of overloads for AtlasProjection._tiles_in_order (since you aren't supposed to return a Union if the return type can be assumed by the caller, and Any seemed too broad for this simple case).

One thing I don't understand is why #292 changed some of the PEP 604 unions to the old-style Optional, but left others alone. If PEP 604 is not allowed in this repo, shouldn't all of the new-style unions be removed? What about PEP 585 syntax, as also used in this file?