Conversation
Co-authored-by: Andres O. Vela <andresovela@users.noreply.github.com>
Co-authored-by: Andres O. Vela <andresovela@users.noreply.github.com>
not be moved into another thread)
| print!("*"); | ||
| if active_threads.is_multiple_of(50) { | ||
| if spawned_sessions.is_multiple_of(50) { | ||
| print!("\n"); |
There was a problem hiding this comment.
Clippy suggested this:
| print!("\n"); | |
| println!(); |
There was a problem hiding this comment.
I wonder why clippy does not complain here in CI and also not locally on my machine 🤔
There was a problem hiding this comment.
It was shown in my editor. On the command line it seems to only be shown when using
cargo clippy --all-targets --all-features
It looks like by default tests/examples/benchmarks are not included?
We should probably change our CI settings to something like
cargo clippy --all-targets --all-features -- -D warnings
Fl1tzi
left a comment
There was a problem hiding this comment.
I will have a closer look tomorrow.
| pub async fn initialize(&self, config: &ProcessorConfig) -> Result<(), AicError> { | ||
| let inner = Arc::clone(&self.inner); | ||
| let config = config.clone(); |
There was a problem hiding this comment.
I'm not sure if cloning the ProcessingConfig is a needed step here. I would assume that we could just accept an owned ProcessorConfig and implement Copy on the ProcessorConfig.
I haven't tried but #[derive(Copy)] should just work because all the types inside the ProcessorConfig implement Copy.
| pub async fn initialize(&self, config: &ProcessorConfig) -> Result<(), AicError> { | |
| let inner = Arc::clone(&self.inner); | |
| let config = config.clone(); | |
| pub async fn initialize(&self, config: ProcessorConfig) -> Result<(), AicError> { | |
| let inner = Arc::clone(&self.inner); |
There was a problem hiding this comment.
That is correct, but I like to have the function similar to the non-async processor.
And in the end copying it outside results in the same as cloning it inside.
But it is a bit more beautiful, I agree.
There was a problem hiding this comment.
@mgeier any preference here? Leave the interface the same in both or change it here to by-value instead of by-reference?
There was a problem hiding this comment.
In general, I think I would prefer the explicit clone() and passing it by reference.
The ProcessorConfig is very small, so implicit copying might be fine for now, but if the config grows over time I think it's better to make cloning explicit.
And another thing: I'm not sure if that's happening in reality, but if we have a mutable ProcessorConfig, we might inadvertently make copies of it and then make changes to different instances, leading to confusion ...
There was a problem hiding this comment.
#51 suggests a way to pass the config by reference without copying (but with using unsafe).
There was a problem hiding this comment.
As another data point, there's currently an open PR at cpal which suggests adding a DuplexStreamConfig which is Copy:
Here is a comment regarding by value vs. by reference: RustAudio/cpal#1096 (review).
I think my argument would be the same, and I would advise against Copy.
Co-authored-by: Matthias Geier <Matthias.Geier@gmail.com>
| let model_path = Model::download(MODEL, "target")?; | ||
| let model = Model::from_file(&model_path)?; |
There was a problem hiding this comment.
I think ideally the model downloading and loading should also be async as in a full async setup, you would need to spawn a blocking task here again.
aic-sdk-rs/src/download/mod.rs
Lines 35 to 40 in d9a4d58
Line 86 in d9a4d58
Co-authored-by: Matthias Geier <Matthias.Geier@gmail.com>
No description provided.