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

Introduce EmbeddedDocument #101

Closed

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Oct 2, 2024

@OskarStark OskarStark self-assigned this Oct 2, 2024
@OskarStark OskarStark changed the base branch from main to feature/text-required-document October 2, 2024 11:25
@OskarStark OskarStark changed the title feature/add embedded document Introduce EmbeddedDocument Oct 2, 2024
@OskarStark OskarStark changed the title Introduce EmbeddedDocument Introduce EmbeddedDocument Oct 2, 2024
@chr-hertel
Copy link
Member

what do you think about like a tupel of document classes:

TextDocument and VectorDocument - both have an UUID and Metadata

/**
 * @param TextDocument|TextDocument[] $documents
 */
DocumentEmbedder::create(TextDocument|array $documents, ...): void

// only converting than

$vectorDocuments[] = new VectorDocument($vectors[$i], $document->id, $document->metadata);

// and drop text in store context completely

/**
 * @param VectorDocument|VectorDocument[] $documents
 **/
StoreInterface::add(VectorDocument|array $documents): void;

/**
 * @return VectorDocument[]
 */
VectorStoreInterface::query(Vector $vector, array $options = []): array;

@OskarStark
Copy link
Contributor Author

I like it, I thought sth like this without inheritance too in the beginning

@chr-hertel
Copy link
Member

thought about a Document interface with getId(): Uuid and getMetadata(): Metadata; - but i don't see a user of that interface in our code :D

@OskarStark
Copy link
Contributor Author

No I don't think we need that abstraction upstream

@chr-hertel
Copy link
Member

you want to tackle that with your PRs or shall I?

@OskarStark
Copy link
Contributor Author

If you have time, go ahead. Either use my PR or open a new one where you do the transformation completely

@chr-hertel chr-hertel force-pushed the feature/text-required-document branch from 1a57ca7 to f7107fa Compare October 3, 2024 20:58
@OskarStark
Copy link
Contributor Author

@OskarStark OskarStark closed this Oct 3, 2024
@chr-hertel chr-hertel deleted the feature/add-embedded-document branch October 12, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants