chroma-core / chroma

the AI-native open-source embedding database
https://www.trychroma.com/
Apache License 2.0
13.36k stars 1.14k forks source link

[ENH]: Static re-export of existing EFs for IDE static analysis #2408

Open tazarov opened 4 days ago

tazarov commented 4 days ago

Closes #2398

Description of changes

Summarize the changes made by this PR.

Test plan

How are these changes tested?

Documentation Changes

github-actions[bot] commented 4 days ago

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

tazarov commented 4 days ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tazarov and the rest of your teammates on Graphite Graphite

tazarov commented 4 days ago

@atroyn, should we create a small guide about creating/adding new EFs, which should include:

atroyn commented 4 days ago

This just reverts us to static imports, which is undesirable. Did we explore any alternatives ?

tazarov commented 4 days ago

@atroyn, we can generate a .pyi file when packaging, but cloning from main won't work as the .pyi file won't exist. We can also add the stub (pyi) generation as part of the merge to the main when the ef sub dir changes; it is slightly hacky but can work.

A benefit of dynamic imports I see is that it makes adding new EFs ever so slightly simple, by removing the need to re-export from __init__. However, implementing dynamic imports that work with IDEs seems like a much taller (as per the above) order than having everybody statically re-export (one-liner). But it is likely I am missing a bigger point here.

How others are doing it:

atroyn commented 4 days ago

I think we will eventually move to a separate contrib package which should help here (I think?). I think you're right that we should prefer the user experience to the developer experience.

One idea is this; we can use the dynamic import as a check to see if you created a new EF in the module, but did not import it yet, to remind you to import it. This could actually be a test or linting step, and there could be a decorator to ignore specific ones.

That might be best of both worlds.