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

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

chiruu12
Copy link

@chiruu12 chiruu12 commented Feb 26, 2025

close #177

@github-actions github-actions bot added the llm label Feb 26, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Feb 26, 2025
@chiruu12

This comment was marked as outdated.

@imbajin

This comment was marked as outdated.

@chiruu12

This comment was marked as outdated.

@imbajin

This comment was marked as outdated.

@chiruu12
Copy link
Author

@imbajin thx for the suggestion will do so!

@chiruu12

This comment was marked as outdated.

@imbajin
Copy link
Member

imbajin commented Feb 27, 2025

@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

@chiruu12

This comment was marked as outdated.

@imbajin

This comment was marked as outdated.

@chiruu12

This comment was marked as outdated.

@chiruu12
Copy link
Author

@imbajin shall I commit the changes that I made?

@imbajin

This comment was marked as outdated.

@chiruu12
Copy link
Author

@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

yield f"data: {error_data}\n\n"

return StreamingResponse(
generate_stream(),

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.
yield f"data: {error_data}\n\n"

return StreamingResponse(
generate_graph_stream(),

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.
@imbajin
Copy link
Member

imbajin commented Feb 28, 2025

@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?

@chiruu12
Copy link
Author

chiruu12 commented Feb 28, 2025

@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

@imbajin
Copy link
Member

imbajin commented Feb 28, 2025

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

@chiruu12
Copy link
Author

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
Copy link
Author

chiruu12 commented Mar 2, 2025

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

@imbajin
Copy link
Member

imbajin commented Mar 2, 2025

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

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 6, 2025
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Mar 6, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Mar 6, 2025
@chiruu12

This comment was marked as outdated.

@chiruu12
Copy link
Author

chiruu12 commented Mar 6, 2025

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

@imbajin
Copy link
Member

imbajin commented Mar 6, 2025

@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

PS: Remember pull code first~

Copy link
Member

Choose a reason for hiding this comment

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

Hi, could tell me how u test the APIs? By directly request them?

The gradio UI loss the API link now

Before:
image

Now:
image

Maybe refer here: (Or Gradio's mount doc?)

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 6, 2025
from hugegraph_llm.operators.hugegraph_op.graph_rag_query import GraphRAGQuery
graph_rag = GraphRAGQuery()
graph_rag.init_client(chunk)
vertex_details = await graph_rag.get_vertex_details(chunk["match_vids"])
Copy link
Member

Choose a reason for hiding this comment

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

'await' operator can't be used on list?

@@ -108,7 +110,7 @@ def graph_rag_recall_api(req: GraphRAGRequest):
from hugegraph_llm.operators.hugegraph_op.graph_rag_query import GraphRAGQuery
graph_rag = GraphRAGQuery()
graph_rag.init_client(result)
vertex_details = graph_rag.get_vertex_details(result["match_vids"])
vertex_details = await graph_rag.get_vertex_details(result["match_vids"])
Copy link
Member

Choose a reason for hiding this comment

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

so as here

# pylint: disable=too-many-statements
def rag_http_api(
async def rag_http_api(
Copy link
Member

@imbajin imbajin Mar 6, 2025

Choose a reason for hiding this comment

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

if we use a async api call, seems we should also update the usage in app.py?

image

still not work as expected (Also note admin_api need used in the same way

Copy link
Author

Choose a reason for hiding this comment

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

I will try to resolve the issues and then I will ping you again sir!

@imbajin
Copy link
Member

imbajin commented Mar 7, 2025

@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

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

@chiruu12
Copy link
Author

chiruu12 commented Mar 7, 2025

@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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request llm size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make API layer implementation support async and streaming output mode
2 participants