incubator-hugegraph-ai icon indicating copy to clipboard operation
incubator-hugegraph-ai copied to clipboard

feat(llm): support async/streaming output mode in api layer

Open chiruu12 opened this issue 9 months ago • 27 comments

close #177

chiruu12 avatar Feb 26 '25 04:02 chiruu12

@imbajin Can you please review and tell me if I need to make any changes??

chiruu12 avatar Feb 26 '25 04:02 chiruu12

@imbajin Can you please review and tell me if I need to make any changes??

Thanks, can we add some basic tests to it? You can create it separately first, and we will also enable it in the CI actions

imbajin avatar Feb 26 '25 04:02 imbajin

@imbajin Can you please review and tell me if I need to make any changes??

Thanks, can we add some basic tests to it? You can create it separately first, and we will also enable it in the CI actions

Ok sir will do

chiruu12 avatar Feb 26 '25 04:02 chiruu12

Also need handle the lint error: (see https://github.com/apache/incubator-hugegraph-ai/blob/2ae610c32f7b174cff199926480f9bb3e2e02c02/.github/workflows/pylint.yml#L63 & https://github.com/apache/incubator-hugegraph-ai/blob/main/style/code_format_and_analysis.sh to run it local, could also remind it in the README for newcomers to know it)

image

Another improvement is that the streaming api can be extracted separately into a separate file similar to stream_api.py? It is advisable to separate the functions related to/config into config_api.py

imbajin avatar Feb 26 '25 10:02 imbajin

@imbajin thx for the suggestion will do so!

chiruu12 avatar Feb 26 '25 18:02 chiruu12

@imbajin sir I am having a little problem locating pyhugegraph can you tell me where it is ?

chiruu12 avatar Feb 27 '25 06:02 chiruu12

@imbajin sir I am having a little problem locating pyhugegraph can you tell me where it is ?

U could ignore it (not related to current PR)

BTW, py-client here-> https://github.com/apache/incubator-hugegraph-ai/tree/main/hugegraph-python-client

imbajin avatar Feb 27 '25 07:02 imbajin

@imbajin I will push the changes I have made and shall i also add the test file you asked me to make? I was just working on finishing it up and one concern that I have is how will we know if the api is working fine without testing it sir??

chiruu12 avatar Feb 27 '25 07:02 chiruu12

@imbajin I will push the changes I have made and shall i also add the test file you asked me to make? I was just working on finishing it up and one concern that I have is how will we know if the api is working fine without testing it sir??

Yes, we do need to start testing. The previous section was missing. Can you first refer to the previous tests and add an API test file, and then test it briefly? https://github.com/apache/incubator-hugegraph-ai/tree/main/hugegraph-llm/src/tests

Additionally, it can also be enabled in CI, similar to Python client's tests (https://github.com/apache/incubator-hugegraph-ai/blob/e78792f5b25443e953671c2f752a1be2661499e2/.github/workflows/hugegraph-python-client.yml#L66)

image

imbajin avatar Feb 27 '25 07:02 imbajin

@imbajin yes sir will start doing it

chiruu12 avatar Feb 27 '25 07:02 chiruu12

@imbajin shall I commit the changes that I made?

chiruu12 avatar Feb 27 '25 08:02 chiruu12

@imbajin shall I commit the changes that I made?

Sure, u can submit new changes at any time without any concerns

Also note pylint~ image

imbajin avatar Feb 27 '25 08:02 imbajin

@imbajin I have separated the files for stream_api and config too and tried to make the format better for passing the pylint test too I had to manually do it any way we can do it using a script? whenever i am running the script it is telling me I have to do it manually

chiruu12 avatar Feb 27 '25 18:02 chiruu12

@imbajin I have separated the files for stream_api and config too and tried to make the format better for passing the pylint test too I had to manually do it any way we can do it using a script? whenever i am running the script it is telling me I have to do it manually

Not quite sure what you mean when you say the script content? Should Pytest be able to specify multiple tests at once without having to execute them separately multiple times? Can you provide specific explanations for it?

imbajin avatar Feb 28 '25 03:02 imbajin

@imbajin I have separated the files for stream_api and config too and tried to make the format better for passing the pylint test too I had to manually do it any way we can do it using a script? whenever i am running the script it is telling me I have to do it manually

Not quite sure what you mean when you say the script content? Should Pytest be able to specify multiple tests at once without having to execute them separately multiple times? Can you provide specific explanations for it?

sir I have been going through them manually one by one, which is pretty time-consuming. Is there a better approach I'm missing? Maybe a tool or command that can automatically fix some of the common pylint issues instead of just listing them? I'm not very experienced with pylint and wondered if there's a more efficient way to handle this than the manual corrections I've been doing. Like I have contributed to openvino once and they told me to run a small command using black and then the issue with spacing was resolved automatically

and I saw that there are a few errors which are coming like the FIXME: line 31: E0702: Raising dict while only classes or instances are allowed (raising-bad-type) This error stems from admin_api.py class

chiruu12 avatar Feb 28 '25 03:02 chiruu12

sir I have been going through them manually one by one, which is pretty time-consuming. Is there a better approach I'm missing? Maybe a tool or command that can automatically fix some of the common pylint issues instead of just listing them? I'm not very experienced with pylint and wondered if there's a more efficient way to handle this than the manual corrections I've been doing. Like I have contributed to openvino once and they told me to run a small command using black and then the issue with spacing was resolved automatically

and I saw that there are a few errors which are coming like the FIXME: line 31: E0702: Raising dict while only classes or instances are allowed (raising-bad-type) This error stems from admin_api.py class

If you're saying that repairing pylint is troublesome, you can ignore it for now and I'll help to fix it together. (Focus on your code logic)

As for automated Lint repair tools, I'm not very familiar with them. If there are any suitable ones, could recommend it and introduce them into our project, which would be even better

imbajin avatar Feb 28 '25 04:02 imbajin

sir I have been going through them manually one by one, which is pretty time-consuming. Is there a better approach I'm missing? Maybe a tool or command that can automatically fix some of the common pylint issues instead of just listing them? I'm not very experienced with pylint and wondered if there's a more efficient way to handle this than the manual corrections I've been doing. Like I have contributed to openvino once and they told me to run a small command using black and then the issue with spacing was resolved automatically and I saw that there are a few errors which are coming like the FIXME: line 31: E0702: Raising dict while only classes or instances are allowed (raising-bad-type) This error stems from admin_api.py class

If you're saying that repairing pylint is troublesome, you can ignore it for now and I'll help to fix it together.

As for automated Lint repair tools, I'm not very familiar with them. If there are any suitable ones, could recommend it and introduce them into our project, which would be even better

Thank you so much sir I really appreciate your guidance. I'm still getting up to speed with best practices, but I'm confident about handling the code logic correctly. If any issues arise, I'll make sure to work on them swiftly.

chiruu12 avatar Feb 28 '25 04:02 chiruu12

If you're saying that repairing pylint is troublesome, you can ignore it for now and I'll help to fix it together. (Focus on your code logic)

As for automated Lint repair tools, I'm not very familiar with them. If there are any suitable ones, could recommend it and introduce them into our project, which would be even better

Sir I check the repo I was telling you about shall I share the script that the used? I think they used black and ruff

chiruu12 avatar Mar 02 '25 03:03 chiruu12

Sir I check the repo I was telling you about shall I share the script that the used? I think they used black and ruff

Okay, regarding the issue of lint and automatic formatting, we can create a separate PR or issue for management, and we will keep the current PR independent

imbajin avatar Mar 02 '25 15:03 imbajin

Due to #184 merged, we could resolve the conflicts first image|

Also, could refer here to set the PR title: https://github.com/apache/incubator-hugegraph/wiki/The-style-config-for-HugeGraph-in-IDEA#0x00-github-prgit-processusage

imbajin avatar Mar 03 '25 08:03 imbajin

@imbajin sure sir will do can give me a little guidance on what will be the most effective way I can resolve the conflict?

chiruu12 avatar Mar 03 '25 10:03 chiruu12

@imbajin sure sir will do can give me a little guidance on what will be the most effective way I can resolve the conflict?

Maybe in most cases, it should be to confirm the difference before and after the conflict? Then you can compare the uncertain areas in your IDE.

imbajin avatar Mar 03 '25 11:03 imbajin

@imbajin sir I think the error is not related to my pr ... Screenshot 2025-03-06 084112

chiruu12 avatar Mar 06 '25 03:03 chiruu12

@imbajin Sir please check the pr and let me know if there is any changes that I need to make

chiruu12 avatar Mar 06 '25 09:03 chiruu12

@imbajin Sir please check the pr and let me know if there is any changes that I need to make

Fine, we add a separate async_streaming_generate in the latest change/commit, seems we could use it to provide the aysnc_streaming api?

refer https://github.com/apache/incubator-hugegraph-ai/blob/7ae5d6fcc1013bc39164672355a8270f078bff8d/hugegraph-llm/src/hugegraph_llm/models/llms/base.py#L51

PS: Remember pull code first~

imbajin avatar Mar 06 '25 10:03 imbajin

@imbajin Sir please check the pr and let me know if there is any changes that I need to make

Fine, we add a separate async_streaming_generate in the latest change/commit, seems we could use it to provide the aysnc_streaming api?

refer

https://github.com/apache/incubator-hugegraph-ai/blob/7ae5d6fcc1013bc39164672355a8270f078bff8d/hugegraph-llm/src/hugegraph_llm/models/llms/base.py#L51

PS: Remember pull code first~

@chiruu12 Also note the stream_http_api is also not bounded to gradio, and it seems that the design may need to be improved. (and remember to use agenerate_streaming() function if we can)

And it's better to follow the design of LLM such as OpenAI to provide a stream parameter instead of adding a separate API path. The internal logic can be implemented separately in two functions to provide a unified interface for the user layer

imbajin avatar Mar 07 '25 04:03 imbajin

@imbajin Sir please check the pr and let me know if there is any changes that I need to make

Fine, we add a separate async_streaming_generate in the latest change/commit, seems we could use it to provide the aysnc_streaming api? refer https://github.com/apache/incubator-hugegraph-ai/blob/7ae5d6fcc1013bc39164672355a8270f078bff8d/hugegraph-llm/src/hugegraph_llm/models/llms/base.py#L51

PS: Remember pull code first~

@chiruu12 Also note the stream_http_api is also not bounded to gradio, and it seems that the design may need to be improved. (and remember to use agenerate_streaming() function if we can)

And it's better to follow the design of LLM such as OpenAI to provide a stream parameter instead of adding a separate API path. The internal logic can be implemented separately in two functions to provide a unified interface for the user layer

understood sir I will try to get this done as soon as my exams are over and then maybe we can work on the other issue too! Also had made an elementary demo for the agent. Let me know if you will get some time to review it too.

chiruu12 avatar Mar 07 '25 05:03 chiruu12