Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
implement kornia-dnn with RTDETR detector #129
base: main
Are you sure you want to change the base?
implement kornia-dnn with RTDETR detector #129
Changes from 2 commits
0095739
fc3d72c
1344de3
8193603
3cda7e1
8cd3394
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Execution providers can be registered through the environment so this is super minor but WDYT about an
execution_providers: Vec<ExecutionProviderDispatch>
field &with_execution_providers
to configure EPs specifically for the RTDETR session?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wanted to somehow make ort transparent to the user and avoid them to explicitly pass the
ort::ExecutionProvider
and have something customI'm still experimenting with the execution providers. What is the point of defining multiple as a Vec, just because of a fallback provider ?
I've played a bit with it and i noticed that cuda/tensorrt takes few seconds to run the first frames.
A couple of questions:
commit_from_file
-- the idea is that we will have a bunch of operators/models in our Kornia HF hub which can use thecommit_from_url
but somehow also let the user also to give a local onnx file. Any tips here ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about re-exporting EPs like
pub use ort::{CPUExecutionProvider, CUDAExecutionProvider, TensorRTExecutionProvider}
so users can still configure each EP's options instead of being limited to defaults? That should still keep things neat.Yes.
You could run it on 1 dummy frame inside the constructor, that should get the graph warmed up.
Yes, and for CUDA it is determining the most optimal cuDNN convolution kernels. By default it performs an exhaustive search which gets the best performance at the cost of significant warmup time - this can be configured with
CUDAExecutionProvider::with_conv_algorithm_search
.TensorRT graphs can theoretically be cached with
TensorRTExecutionProvider::with_engine_cache
but some users in the pyke Discord have reported that ONNX Runtime sometimes doesn't respect this option, and session creation can still take a few seconds despite using a cached engine. Your mileage may vary, though; I personally haven't been able to reproduce the issue, but it's just something to keep in mind.How about something like this? (very roughly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually one more idea would be to kinda automatically set based on feature flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@decahedron1 how could we pre-allocate the output tensor here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the perfect use case for
OutputSelector
!