Skip to content

Conversation

@lvchzh
Copy link

@lvchzh lvchzh commented May 30, 2024

Hi, this PR is add Wenxin model client and has passed unit testing.

I can provide my api_key if needed for testing

the PR content:

chat client
embedding client
spring starter
unit test

embedding client documents
For some reasons, products from OpenAI and others can't be directly used in Chinese Mainland.

QianFan is a big model platform, A variety of models are offered, among which ERNIE Speed and ERNIE Lite are fully free. Although their API design is quite strange.

the link: QianFan

If they can supported, it will further aid spring-ai to promotion in Chinese Mainland.

If necessary, I can take care of subsequent maintenance since I'm currently using them.

@lvchzh
Copy link
Author

lvchzh commented Jun 3, 2024

@markpollack
I have recently submitted a pull request (PR) on spring ai, numbered #795, titled “feature:add Wenxin model client ”. This PR is intended to support ERNIE Model
Given your expertise in this area, I would greatly appreciate your feedback. When you have a moment, could you please review this PR? If you notice any issues or have suggestions for improvement, I would be very grateful for your insights.

Thank you for your time and support!

Best regards,

lvchzh

@mxsl-gr
Copy link
Contributor

mxsl-gr commented Jun 3, 2024

Hi, this PR is add Wenxin model client and has passed unit testing.

I can provide my api_key if needed for testing

the PR content:

chat client
embedding
client spring
starter unit test

embedding client documents For some reasons, products from OpenAI and others can't be directly used in Chinese Mainland.

QianFan is a big model platform, A variety of models are offered, among which ERNIE Speed and ERNIE Lite are fully free. Although their API design is quite strange.

the link: QianFan

If they can supported, it will further aid spring-ai to promotion in Chinese Mainland.

If necessary, I can take care of subsequent maintenance since I'm currently using them.

hi, it seems we share an interest in the same area of contribution. I'd be happy to collaborate together, as I'm currently using the snapshot version locally

however, I noticed that you copied even the comments, just tweak the name, from PR #751

by the way, the base model's name is 'WenXin', and the platform is 'QianFan', which you've copied without changing.

anyway, If you're interested in collaborating, feel free to contact me.

@lvchzh
Copy link
Author

lvchzh commented Jun 3, 2024

@mxsl-gr
I have noticed the issue you mentioned, but after carefully reviewing the content of my PR and the commits in #751, it seems that my PR is more comprehensive and detailed. Additionally, I will continue to follow up on updates in subsequent versions. I hope it will be accepted.

@lvchzh
Copy link
Author

lvchzh commented Jun 4, 2024

@tzolov
Could you help review this PR? It's very important to me. Thank you very much.

@lvchzh lvchzh marked this pull request as draft June 5, 2024 02:03
@lvchzh lvchzh marked this pull request as ready for review June 5, 2024 02:04
@lvchzh
Copy link
Author

lvchzh commented Jun 5, 2024

@maxjiang153
I've submitted a pull request for your review. Please let me know if there are any adjustments you'd recommend.

@lvchzh
Copy link
Author

lvchzh commented Jun 18, 2024

@tzolov Conflict has been resolved.

@tzolov
Copy link
Contributor

tzolov commented Jun 18, 2024

Hey @lvchzh how can I test this client?
Seems like I can't register to QianFan without having Chainis mobile?

@lvchzh
Copy link
Author

lvchzh commented Jun 19, 2024

Hey @lvchzh how can I test this client? Seems like I can't register to QianFan without having Chainis mobile?

If needed, I can provide AK/SK.
ACCESS_KEY=0499e5ebd5b44d0ba846a08a1cc467a2
SECRET_KEY=9e0b20a6dda047b2b9bc1a4974b901d8

Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @lvchzh

It looks good so far but there are still some important gaps to be filled in.
Please consider addressing the following issues:

  • rename the ACCESS_KEY and SECRET_KEY environment variables used for ITs to WENXIN_ACCESS_KEY and WENXIN_SECRET_KEY instead.
  • add the <artifactId>spring-ai-wenxin-spring-boot-starter</artifactId> dependency to the BOM's pom
  • Add missing WenxinChatModel integration Tests. You can mimic the OpenAI or MistralAI ITs for example.
  • Remove all empty classes like: WenxinAudioSpeechModel, WenxinAudioSpeechOptions, WenxinAudioTranscriptionModel, WenxinAudioTranscriptionOptions, WenxinImageModel, WenxinImageOptions, WenxinImageProperties, WenxinAudioSpeechProperties , WenxinAudioTranscriptionProperties, .
  • Replace the following javadoc snippet
 * @date 2024年05月14日 下午5:29
 * @description:

with @since 1.0.0.

@lvchzh
Copy link
Author

lvchzh commented Jun 25, 2024

Thanks for your contribution @lvchzh

It looks good so far but there are still some important gaps to be filled in. Please consider addressing the following issues:

  • rename the ACCESS_KEY and SECRET_KEY environment variables used for ITs to WENXIN_ACCESS_KEY and WENXIN_SECRET_KEY instead.
  • add the <artifactId>spring-ai-wenxin-spring-boot-starter</artifactId> dependency to the BOM's pom
  • Add missing WenxinChatModel integration Tests. You can mimic the OpenAI or MistralAI ITs for example.
  • Remove all empty classes like: WenxinAudioSpeechModel, WenxinAudioSpeechOptions, WenxinAudioTranscriptionModel, WenxinAudioTranscriptionOptions, WenxinImageModel, WenxinImageOptions, WenxinImageProperties, WenxinAudioSpeechProperties , WenxinAudioTranscriptionProperties, .
  • Replace the following javadoc snippet
 * @date 2024年05月14日 下午5:29
 * @description:

with @since 1.0.0.

@tzolov
hi,Thank you very much for your feedback and suggestions! Regarding the issues mentioned, I have arranged the following:

Environment Variable Naming: I have renamed the ACCESS_KEY and SECRET_KEY environment variables to WENXIN_ACCESS_KEY and WENXIN_SECRET_KEY to better reflect their purpose and avoid potential naming conflicts.

Dependency Addition: I have added the spring-ai-wenxin-spring-boot-starter dependency to the BOM's pom file to ensure the Wenxin module can be correctly integrated into the project.

Integration Tests: Referring to the integration tests (ITs) of OpenAI and MistralAI, I have added the missing WenxinChatModel integration tests to ensure the module's functionality and stability.

Removing Empty Classes: All mentioned empty classes such as WenxinAudioSpeechModel, WenxinAudioSpeechOptions, etc., have been removed from the project to clean up the codebase and reduce maintenance burdens.

Javadoc Annotation: I have changed the Javadoc date description in all files to @SInCE 1.0.0 to indicate that this feature has been introduced since version 1.0.0.

Additional Integration Tests: Integration tests have been added for the WenxinAutoConfiguration to ensure the accuracy of its auto-configuration.

Documentation Addition: I have added documentation about the Wenxin module in the official Spring AI documentation, including usage methods, configuration options, and sample codes, following the structure used in other API documents in the spring-ai project.

The above adjustments have been pushed to the main branch, and I look forward to your further review and feedback. If there are any other areas that need adjustment, please let me know anytime!

@lvchzh lvchzh requested a review from tzolov June 25, 2024 02:36
@lvchzh
Copy link
Author

lvchzh commented Jul 25, 2024

Thanks for your contribution @lvchzh

It looks good so far but there are still some important gaps to be filled in. Please consider addressing the following issues:

  • rename the ACCESS_KEY and SECRET_KEY environment variables used for ITs to WENXIN_ACCESS_KEY and WENXIN_SECRET_KEY instead.
  • add the <artifactId>spring-ai-wenxin-spring-boot-starter</artifactId> dependency to the BOM's pom
  • Add missing WenxinChatModel integration Tests. You can mimic the OpenAI or MistralAI ITs for example.
  • Remove all empty classes like: WenxinAudioSpeechModel, WenxinAudioSpeechOptions, WenxinAudioTranscriptionModel, WenxinAudioTranscriptionOptions, WenxinImageModel, WenxinImageOptions, WenxinImageProperties, WenxinAudioSpeechProperties , WenxinAudioTranscriptionProperties, .
  • Replace the following javadoc snippet
 * @date 2024年05月14日 下午5:29
 * @description:

with @since 1.0.0.

@tzolov Can you help me review the code? If everything looks good, could you assist in merging it?

@ThomasVitale
Copy link
Contributor

@lvchzh Thanks for your contributions. If I understood correctly, the Wenxin/ERNIE models are available through the Baidu Qianfan platform. Is a dedicated ChatModel implementation needed or can the existing Qianfan module contributed by @mxsl-gr be used instead? https://github.com/spring-projects/spring-ai/tree/main/models/spring-ai-qianfan

If I look at the LangChain project, they have a single Baidu Qianfan module for the different supported models: https://python.langchain.com/v0.1/docs/integrations/llms/baidu_qianfan_endpoint/

@lvchzh
Copy link
Author

lvchzh commented Aug 3, 2024

@lvchzh Thanks for your contributions. If I understood correctly, the Wenxin/ERNIE models are available through the Baidu Qianfan platform. Is a dedicated ChatModel implementation needed or can the existing Qianfan module contributed by @mxsl-gr be used instead? https://github.com/spring-projects/spring-ai/tree/main/models/spring-ai-qianfan

If I look at the LangChain project, they have a single Baidu Qianfan module for the different supported models: https://python.langchain.com/v0.1/docs/integrations/llms/baidu_qianfan_endpoint/
First, I am sorry to receive this news. When creating this submission, I was unaware that other developers had already created module code for the related models. Additionally, the process from code submission to review took a long time, and during this period, some feedback was left waiting for quite some time. After receiving the review suggestions, I made the necessary modifications at the earliest opportunity, but it seems there hasn't been an active response since then.

As a developer, I was very excited when I first encountered this open-source project. It was inspiring to see a Java framework based on LLMs, and I approached the entire framework with great enthusiasm, conducting an in-depth study of its implementation. Based on the models I intended to use, I constructed an integration module by referring to the implementations of models supported by the framework.

Regarding your mention of a potential duplication issue between submission #795 and this submission, I believe the connection is minimal, as there are fundamental differences in the model integration methods. There is no essential conflict between this submission and #795. Personally, I still hope to be accepted; if not, I will be very disappointed.

Lastly, I want to thank you and the other developers for the feedback on the issues encountered during the submission process.

@ThomasVitale
Copy link
Contributor

@lvchzh thanks for your answer! I'm working on instrumenting Spring AI for observability, so I was trying to understand better the common traits among the different ChatModel implementations, both existing and under development, and make sure to surface the right information through the observation APIs.

@lvchzh
Copy link
Author

lvchzh commented Aug 3, 2024

@lvchzh thanks for your answer! I'm working on instrumenting Spring AI for observability, so I was trying to understand better the common traits among the different ChatModel implementations, both existing and under development, and make sure to surface the right information through the observation APIs.

Okay, I understand your concerns. I will also continue to submit the relevant code.

@markpollack
Copy link
Member

@lvchzh It is very difficult for us to accept and maintain models that aren't easily available for testing/development and tend to have docs in chinese only. We did create the Spring AI Community github organization for just such a purpose.

You can read more about it here - https://github.com/spring-ai-community

sorry for letting this PR hand out so long. If you are interested in developing and maintaining this model client, please submit an issue in https://github.com/spring-ai-community/community/issues/

@markpollack markpollack closed this May 3, 2025
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.

5 participants