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

refactor and pybind of OnlineWebsocketServer #1943

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

manickavela29
Copy link
Contributor

@manickavela29 manickavela29 commented Feb 28, 2025

Hi @csukuangfj ,

I have refactored the previous implementation,
Below code base is functional tested and cleaned up

I think you can suggest more for naming for more clarity
or clean up.

overall summary,
-- refactoring online-websocket-server
-- pybind interface
-- signal termination, graceful shut down

Thanks

closed #1931

@manickavela29
Copy link
Contributor Author

Hi @csukuangfj,
could you approve for workflows, meanwhile you get time into review,
it will give me ample time to fix any failures

@csukuangfj
Copy link
Collaborator

Have you compiled and tested it locally?

Does it build successfully and also run successfully for you?

@manickavela29
Copy link
Contributor Author

yes, it is functional in my local testing with CUDA provider,

when I raised PR, observed that for some android build, it was failing with '#include "asio"',
so wanted to fix such issues meanwhile

@csukuangfj
Copy link
Collaborator

it was failing with '#include "asio"',

In that case, I suggest that you move the online-websocket-server.cc to sherpa-onnx/python/csrc.
It is better to not make sherpa-onnx-core contain websocket related stuff.

note that move there means you can change sherpa-onnx/python/csrc/CMakeLists.txt to include that .cc file.
You don't need to move that file physically to that folder.

@manickavela29
Copy link
Contributor Author

I have moved the WebSocket file to python/src object file and cleaned it, let me know if it is good enough

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.

2 participants