Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core[minor]: Make VectorStore generic and pass Metadata type to DocumentInterface #4590

Closed
wants to merge 0 commits into from

Conversation

davidfant
Copy link
Contributor

Followup on #4568

Copy link

vercel bot commented Feb 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 13, 2024 10:43pm
langchainjs-docs ✅ Ready (Inspect) Visit Preview Mar 13, 2024 10:43pm

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. auto:improvement Medium size change to existing code to handle new use-cases labels Feb 29, 2024
Copy link
Member

@bracesproul bracesproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple questions, thank you for doing this!

Metadata extends Record<string, any> = Record<string, any>
> extends BaseRetrieverInterface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Metadata type should be passed to BaseRetrieverInterface too

Suggested change
Metadata extends Record<string, any> = Record<string, any>
> extends BaseRetrieverInterface {
Metadata extends Record<string, any> = Record<string, any>
> extends BaseRetrieverInterface<Metadata> {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this is also missing here

If you could push up a commit updating that to, that would be great!

Comment on lines 230 to 229
export abstract class VectorStore<Metadata extends Record<string, any> = Record<string, any>>
extends Serializable
implements VectorStoreInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing generic getting passed to VectorStoreInterface here too

@bracesproul bracesproul added the in progress PRs that are in progress but not ready to merge label Mar 1, 2024
@bracesproul bracesproul changed the title Make VectorStore generic and pass Metadata type to DocumentInterface core[minor]: Make VectorStore generic and pass Metadata type to DocumentInterface Mar 1, 2024
@jacoblee93
Copy link
Collaborator

It's having a bad time with the build :(

@davidfant
Copy link
Contributor Author

@jacoblee93 @bracesproul updated!

@jacoblee93
Copy link
Collaborator

Bump @davidfant, I can make this change too

@davidfant
Copy link
Contributor Author

davidfant commented Mar 12, 2024

@jacoblee93 commented
Feel free to make the change and merge if you want, but I don't think it's needed

@jacoblee93
Copy link
Collaborator

Nope you're correct

@jacoblee93
Copy link
Collaborator

jacoblee93 commented Mar 12, 2024

Ok yeah now it's having a bad time with the concrete classes:

src/vectorstores/zep.ts:61:14 - error TS2417: Class static side 'typeof ZepVectorStore' incorrectly extends base class static side 'typeof VectorStore'.
  The types returned by 'fromDocuments(...)' are incompatible between these types.
    Type 'Promise<ZepVectorStore>' is not assignable to type 'Promise<VectorStore<Metadata>>'.
      Type 'ZepVectorStore' is not assignable to type 'VectorStore<Metadata>'.
        The types returned by 'similaritySearchVectorWithScore(...)' are incompatible between these types.
          Type 'Promise<[Document<Record<string, any>>, number][]>' is not assignable to type 'Promise<[DocumentInterface<Metadata>, number][]>'.

61 export class ZepVectorStore extends VectorStore {

I can try to untangle later but we might need to find a way to restrict the type

(this is from running yarn build in libs/langchain-community)

@davidfant
Copy link
Contributor Author

@jacoblee93 I fixed lint and build issues now. Both core and community build

@langchain/scripts:build: cache hit, replaying output fc193b026c812d9b
@langchain/scripts:build: Tree shaking checks passed!
@langchain/core:build: cache miss, executing a0fe95693bcccf13
@langchain/core:build: Tree shaking checks passed!
@langchain/openai:build: cache miss, executing ddbe6791a90fc618
@langchain/openai:build: • Packages in scope: @langchain/core
@langchain/openai:build: • Running build in 1 packages
@langchain/openai:build: • Remote caching disabled
@langchain/openai:build: @langchain/scripts:build: cache hit, replaying output fc193b026c812d9b
@langchain/openai:build: @langchain/scripts:build: Tree shaking checks passed!
@langchain/openai:build: @langchain/core:build: cache hit, replaying output a0fe95693bcccf13
@langchain/openai:build: 
@langchain/openai:build:  Tasks:    2 successful, 2 total
@langchain/openai:build: Cached:    2 cached, 2 total
@langchain/openai:build:   Time:    4.321s >>> FULL TURBO
@langchain/openai:build: 
@langchain/openai:build: Tree shaking checks passed!
@langchain/anthropic:build: cache miss, executing 1615c8d45caa9e71
@langchain/anthropic:build: • Packages in scope: @langchain/core
@langchain/anthropic:build: • Running build in 1 packages
@langchain/anthropic:build: • Remote caching disabled
@langchain/anthropic:build: @langchain/scripts:build: cache hit, replaying output fc193b026c812d9b
@langchain/anthropic:build: @langchain/scripts:build: Tree shaking checks passed!
@langchain/anthropic:build: @langchain/core:build: cache hit, replaying output a0fe95693bcccf13
@langchain/anthropic:build: 
@langchain/anthropic:build:  Tasks:    2 successful, 2 total
@langchain/anthropic:build: Cached:    2 cached, 2 total
@langchain/anthropic:build:   Time:    1.972s >>> FULL TURBO
@langchain/anthropic:build: 
@langchain/anthropic:build: Tree shaking checks passed!
@langchain/community:build: cache miss, executing 415773951b0accb7
@langchain/community:build: • Packages in scope: @langchain/anthropic, @langchain/core, @langchain/openai
@langchain/community:build: • Running build in 3 packages
@langchain/community:build: • Remote caching disabled
@langchain/community:build: @langchain/scripts:build: cache hit, replaying output fc193b026c812d9b
@langchain/community:build: @langchain/scripts:build: Tree shaking checks passed!
@langchain/community:build: @langchain/core:build: cache hit, replaying output a0fe95693bcccf13
@langchain/community:build: @langchain/anthropic:build: cache hit, replaying output 1615c8d45caa9e71
@langchain/community:build: @langchain/openai:build: cache hit, replaying output ddbe6791a90fc618
@langchain/community:build: 
@langchain/community:build:  Tasks:    4 successful, 4 total
@langchain/community:build: Cached:    4 cached, 4 total
@langchain/community:build:   Time:    2.318s >>> FULL TURBO
@langchain/community:build: 
@langchain/community:build: Tree shaking checks passed!

@jacoblee93
Copy link
Collaborator

Thank you!

@jacoblee93 jacoblee93 added lgtm PRs that are ready to be merged as-is and removed in progress PRs that are in progress but not ready to merge labels Mar 14, 2024
@jacoblee93
Copy link
Collaborator

Seems like it's failing examples/ build:

Error: examples:build: src/use_cases/sql/agents/example_selector.ts(20,3): error TS2345: Argument of type 'typeof HNSWLib' is not assignable to parameter of type 'typeof VectorStore'.
Error: examples:build: src/use_cases/sql/prompting/dynamic_few_shot.ts(10,3): error TS2344: Type 'typeof MemoryVectorStore' does not satisfy the constraint 'typeof VectorStore'.

I can have a look tomorrow

@davidfant davidfant closed this Mar 22, 2024
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases lgtm PRs that are ready to be merged as-is size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants