Skip to content

Conversation

@sobychacko
Copy link
Contributor

@sobychacko sobychacko commented Dec 7, 2024

  • Replace configuration object with fluent builder pattern
  • Move Milvus-related classes to dedicated milvus package
  • Deprecate MilvusVectorStoreConfig in favor of builder
  • Update constructor to use builder internally
  • Maintain backward compatibility with deprecated config
  • Add comprehensive builder methods with validation

@ilayaperumalg ilayaperumalg self-assigned this Dec 10, 2024
@ilayaperumalg ilayaperumalg added this to the 1.0.0-M5 milestone Dec 10, 2024
.withMetadataFieldName(properties.getMetadataFieldName())
.withEmbeddingFieldName(properties.getEmbeddingFieldName())
return MilvusVectorStore.builder(milvusClient)
.embeddingModel(embeddingModel)
Copy link
Member

Choose a reason for hiding this comment

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

Except isInitializeSchema, rest of the configuration properties don't seem to be set via builder.

Copy link
Member

Choose a reason for hiding this comment

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

We also need a test to validate this case in our AutoConfiguration IT for Milvus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilayaperumalg Created this issue to track this: #1903. Maybe we can tackle this as a separate issue outside the scope of the builder refactoring?

- Replace configuration object with fluent builder pattern
- Move Milvus-related classes to dedicated milvus package
- Deprecate MilvusVectorStoreConfig in favor of builder
- Update constructor to use builder internally
- Maintain backward compatibility with deprecated config
- Add comprehensive builder methods with validation
@sobychacko sobychacko force-pushed the milvus-vector-store-builder branch from 2b0b504 to b22cbb8 Compare December 11, 2024 00:08
@markpollack
Copy link
Member

merged in 218c967

1 similar comment
@markpollack
Copy link
Member

merged in 218c967

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.

3 participants