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

Submit streaming code and examples #42

Closed
wants to merge 6 commits into from

Conversation

ghjdegithub
Copy link
Contributor

No description provided.

@thewh1teagle
Copy link
Owner

thewh1teagle commented Oct 29, 2024

@ghjdegithub

Although it's direct bindings to sherpa when it's possible I prefer to simplify the interface to sherpa onnx even more while it's possible to keep the flexibility. Hope that make sense.
Also see the review comments

Also try to keep only critical comments. Users should work alongside sherpa onnx docs so we don't need to maintain them

@ghjdegithub
Copy link
Contributor Author

@ghjdegithub

Although it's direct bindings to sherpa when it's possible I prefer to simplify the interface to sherpa onnx even more while it's possible to keep the flexibility. Hope that make sense.

Also see the review comments

Also try to keep only critical comments. Users should work alongside sherpa onnx docs so we don't need to maintain them

The first step is to wrap sherpa's unsafe code in a rust-safe structure, which is also safe to expose, and then that safe structure in again wrapping this higher level structure that you're talking about. And then it will continue to encapsulate a little bit more structure like your ziptransform.

@thewh1teagle
Copy link
Owner

@ghjdegithub

Please create another PR with same changes not from your main branch. I can't edit PR's that comes from main branch of others.

@thewh1teagle
Copy link
Owner

The first step is to wrap sherpa's unsafe code in a rust-safe structure, which is also safe to expose, and then that safe structure in again wrapping this higher level structure that you're talking about. And then it will continue to encapsulate a little bit more structure like your ziptransform.

The first step is to wrap sherpa's unsafe code in a rust-safe structure, which is also safe to expose, and then that safe structure in again wrapping this higher level structure that you're talking about. And then it will continue to encapsulate a little bit more structure like your ziptransform.

Can you describe it in pseudo code? What's the benefits over the current approach given the fact that the codebase is small currently?

@thewh1teagle
Copy link
Owner

Moving to #43

@ghjdegithub
Copy link
Contributor Author

The first step is to wrap sherpa's unsafe code in a rust-safe structure, which is also safe to expose, and then that safe structure in again wrapping this higher level structure that you're talking about. And then it will continue to encapsulate a little bit more structure like your ziptransform.

The first step is to wrap sherpa's unsafe code in a rust-safe structure, which is also safe to expose, and then that safe structure in again wrapping this higher level structure that you're talking about. And then it will continue to encapsulate a little bit more structure like your ziptransform.

Can you describe it in pseudo code? What's the benefits over the current approach given the fact that the codebase is small currently?

First, higher-level encapsulation relies only on secure rust code. Second, the rust structure that continues to be encapsulated by unsafe code can provide maximum extensibility to the outside world. You can see that my onlinestream has made a special encapsulation, which can call one function after the caller, but the other function cannot be called, and this operation is prohibited directly in the compilation layer, without documentation.

@ghjdegithub
Copy link
Contributor Author

ghjdegithub commented Oct 29, 2024

@thewh1teagle
It's best not to rely on unsafe code for high level abstractions, it's very unrusty. e.g. your zipformer relies directly on unsafe code, which is very unsafe and unrusty.

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