Skip to content

Conversation

@ntamas92
Copy link
Contributor

@ntamas92 ntamas92 commented Nov 6, 2023

No description provided.

@ntamas92 ntamas92 requested a review from a team November 6, 2023 11:40
@pfmark
Copy link
Contributor

pfmark commented Nov 6, 2023

I'm not a big fan of the Search Index naming as this can be confused with general indexes. Let's Embedding Indexes

@ntamas92
Copy link
Contributor Author

ntamas92 commented Nov 6, 2023

@pfmark Good point, renamed!

@ntamas92 ntamas92 changed the title Expose search indexes of Dataset Expose embedding indexes of Dataset Nov 6, 2023
status: IndexStatus
index_type: IndexType
index_level: IndexLevel
embedding_type: str
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to create an enum for this one as well? I was hesitant because it contains valid values like EfficientNetB7 but also arbitrary ones like unknown

Copy link
Contributor

Choose a reason for hiding this comment

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

we can try w/o it for now

Copy link
Contributor

@jean-lucas jean-lucas left a comment

Choose a reason for hiding this comment

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

lgtm

CHANGELOG.md Outdated

### Added
- Allow direct embedding vector upload together with dataset items. `DatasetItem` now has an additional parameter called `embedding_info` which can be used to directly upload embeddings when a dataset is uploaded.
- Added `dataset.search_indexes` property, which exposes information about every search index which belongs to the dataset.
Copy link
Contributor

Choose a reason for hiding this comment

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

is it embedding_indexes now or search_indexes?

Copy link
Contributor

Choose a reason for hiding this comment

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

should also be embedding_indexes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, thanks!

Comment on lines +29 to +30
@dataclass
class EmbeddingIndex:
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 👍

status: IndexStatus
index_type: IndexType
index_level: IndexLevel
embedding_type: str
Copy link
Contributor

Choose a reason for hiding this comment

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

we can try w/o it for now

@ntamas92 ntamas92 merged commit dc4a025 into master Nov 7, 2023
@ntamas92 ntamas92 deleted the tamasn/expose_search_indexes branch November 7, 2023 09:14
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.

4 participants