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

Added MimeTypeConverters class #1022

Closed
wants to merge 3 commits into from
Closed

Conversation

rozza
Copy link
Contributor

@rozza rozza commented Jul 10, 2024

Allows the converters to be shared between the autoconfigure codebase and the vector-stores code.

Reenabled the vector-stores code and fixed the TestApplication (so that it uses the MimeTypeConverters).

Annotated tests to check for the OPEN_API_KEY environment variable.
Updated to mongodb/atlas:v1.24.0 in vector-stores so inline with the autoconfigure
test codebase.

Related to #698 and the auto configure fix in a1afb0d

Allows the converters to be shared between the autoconfigure codebase
and the vector-stores code.

Reenabled the vector-stores code and fixed the TestApplication
(so that it uses the MimeTypeConverters).

Also annotated tests to check for the `OPEN_API_KEY` environment
variable.

Related to spring-projects#698 and the auto configure fix in a1afb0d
@markpollack
Copy link
Member

I had made a commit for #698 and I somehow missed this.

Aside from where the converters should live (in a separate class or not) the bigger issue I found was that

@AutoConfiguration(after = MongoDataAutoConfiguration.class)
@ConditionalOnClass({ MongoDBAtlasVectorStore.class, EmbeddingModel.class, MongoTemplate.class })
@EnableConfigurationProperties(MongoDBAtlasVectorStoreProperties.class)
public class MongoDBAtlasVectorStoreAutoConfiguration {

The use of after in the autoconfiguration is what was leading to the converter not being replaced. Removing that fixed the overall issue and no need to use BeanPostProcessor I am inquiring if that config should use before. @rozza would you be able to verify that?

@markpollack markpollack modified the milestones: 1.0.0-M2, 1.0.0-RC1 Jul 22, 2024
@rozza
Copy link
Contributor Author

rozza commented Aug 7, 2024

The use of after in the autoconfiguration is what was leading to the converter not being replaced. Removing that fixed the overall issue and no need to use BeanPostProcessor I am inquiring if that config should use before. @rozza would you be able to verify that?

Hi @markpollack - verified using @AutoConfiguration(before = MongoDataAutoConfiguration.class) works. I've updated the PR.

I'm not sure where the converters should live, I opted to move them into the org.springframework.ai.vectorstore.convert namespace so that they could be reused by the TestApplication which doesn't have dependency on rg.springframework.ai.autoconfigure.vectorstore.mongo. Not sure what is the correct idiomatic path forward here.

@sobychacko
Copy link
Contributor

@rozza, I missed the existence of this PR while fixing the MongoDB vector store test failures. I think we can still take some ideas in this PR and refactor some code duplications. For now, I will close this PR, and we can consider opening a new one on top of the changes from #1255. Thanks!

@sobychacko sobychacko closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants