-
Notifications
You must be signed in to change notification settings - Fork 844
feat(inspect): inference query #3156
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(inspect): inference query #3156
Conversation
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.
Pull request overview
This PR refactors inference functionality to improve performance by preventing unnecessary re-renders in virtualized list items. It introduces a custom query hook for retrieving predictions, splits the inference provider into a dedicated opacity provider, and moves inference logic from a context provider to a query-based approach.
Key changes:
- Replaced mutation-based inference with a query-based approach using
useMediaItemInferencehook - Created
InferenceOpacityProviderto manage only opacity state, separated from inference data - Removed the original
InferenceProviderthat combined inference logic with opacity management
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
inspect.tsx |
Removed InferenceProvider wrapper from the main inspect route |
utils.ts |
Added isNonEmptyString utility function for type-safe string validation |
main-content.test.tsx |
Removed InferenceProvider wrapper from test setup |
inference-provider.component.tsx |
Deleted file containing the original combined inference and opacity provider |
inference-opacity-provider.component.tsx |
New file implementing a focused provider for opacity state management only |
media-preview.component.tsx |
Integrated useMediaItemInference hook and passed inference result as props |
inference-result.component.tsx |
Refactored to receive inference result as prop instead of from context |
inference-opacity.component.tsx |
Updated to use new opacity-only provider and receive disabled state as prop |
util.tsx |
Extracted downloadImageAsFile helper function for reuse |
use-media-item-inference.test.tsx |
Added comprehensive test coverage for the new inference query hook |
use-media-item-inference.hook.tsx |
Implemented query-based inference hook using React Query |
dataset-list.component.tsx |
Added InferenceOpacityProvider wrapper around MediaPreview component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...i/src/features/inspect/dataset/media-preview/inference-result/inference-result.component.tsx
Outdated
Show resolved
Hide resolved
...cation/ui/src/features/inspect/dataset/media-preview/hooks/use-media-item-inference.hook.tsx
Outdated
Show resolved
Hide resolved
| queryFn: isNonEmptyString(selectedModelId) | ||
| ? async () => { |
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.
Could you change the order of this ternary to be,
isNonEmtpyString(selectedModelId) === false ? skipToken : async () => {
// code
}
the reason I like this approach more is that it is easier to see when skipToken is used (now I had to scroll all the way to the top to see when it would be returned).
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.
updated
Signed-off-by: Colorado, Camilo <[email protected]>
Signed-off-by: Colorado, Camilo <[email protected]>
f0be2eb to
dd50c8f
Compare
Signed-off-by: Colorado, Camilo <[email protected]>
dd50c8f to
39e9729
Compare
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...cation/ui/src/features/inspect/dataset/media-preview/hooks/use-media-item-inference.hook.tsx
Show resolved
Hide resolved
652e07e
into
open-edge-platform:feature/geti-inspect
📝 Description
This PR adds a custom query to retrieve predictions and refactors the
inference-provider.component.tsx, renaming it toinference-opacity-provider.component.tsxand delegating opacity control to it only. This helps avoid unnecessary re-renders in the virtualized list items. #3148 (comment)✨ Changes
Select what type of change your PR is:
✅ Checklist
Before you submit your pull request, please make sure you have completed the following steps:
For more information about code review checklists, see the Code Review Checklist.