feat(inspect): Export model dialog #3152
Conversation
Signed-off-by: Dmitry Kalinin <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds an export functionality for completed models in the inspect feature, allowing users to download models in various formats (OpenVINO, ONNX, PyTorch) with optional compression for OpenVINO models.
Key Changes:
- Added export model dialog with format and compression selection
- Integrated export action into the model actions menu
- Implemented blob download utility for file downloads
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| application/ui/src/features/inspect/utils.ts | Added downloadBlob utility function for triggering file downloads |
| application/ui/src/features/inspect/models/model-actions-menu.component.tsx | Added export menu item and dialog integration for completed models |
| application/ui/src/features/inspect/models/export-model-dialog.component.tsx | New dialog component for selecting export format and compression options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| if (response.error) { | ||
| throw new Error('Export failed'); |
There was a problem hiding this comment.
The error message 'Export failed' is generic and doesn't provide helpful information. Consider including the actual error details from response.error to help with debugging.
application/ui/src/features/inspect/models/export-model-dialog.component.tsx
Outdated
Show resolved
Hide resolved
| { id: 'int8', name: 'INT8' }, | ||
| { id: 'int8_ptq', name: 'INT8 PTQ' }, | ||
| { id: 'int8_acq', name: 'INT8 ACQ' }, |
There was a problem hiding this comment.
I think here we need to figure out if we can forward the user to some documentation page to explain the differences between these.
Ideally we should tell the user why they should pick one of these options over the other.
There was a problem hiding this comment.
This is the first implementation of this dialog. Ideally, it should be a separate page, so there is plenty of room for improvement in future PRs. I'll consider how it can be implemented neatly.
|
|
||
| const blob = response.data as Blob; | ||
| const compressionSuffix = compression ? `_${compression}` : ''; | ||
| const filename = `${model.name}_${selectedFormat}${compressionSuffix}.zip`; |
There was a problem hiding this comment.
Should we sanitize model.name here? I think at the moment we always use the snake case format from the server, but I can imagine us changing that later on to be "Title case"?
One other thing we could do is prefix with the project name or id (I think name would be more ux friendly) and later model version (in case the user has trained say Padim twice).
Alternatively we could add the export date to the file.
These are mostly suggestions because I find that in Geti I would often export multiple models at a time but have no clue which zip file corresponds to which project/model.
There was a problem hiding this comment.
Good point. For now, I've added the project name. Would it be more helpful to add the train job timestamp to the file than the export timestamp?
| const [isLogsDialogOpen, setIsLogsDialogOpen] = useState(false); | ||
| const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false); | ||
| const [isExportDialogOpen, setIsExportDialogOpen] = useState(false); |
There was a problem hiding this comment.
Refactor suggestion: instead of having multiple isXOpen states, we could have a useState<null | 'logs' | 'delete' | 'export'> type of state?
Signed-off-by: Dmitry Kalinin <[email protected]>
Signed-off-by: Dmitry Kalinin <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
application/ui/src/features/inspect/models/export-model-dialog.component.tsx
Show resolved
Hide resolved
application/ui/src/features/inspect/models/export-model-dialog.component.tsx
Outdated
Show resolved
Hide resolved
| const handleExport = async () => { | ||
| setIsExporting(true); |
There was a problem hiding this comment.
Wdyt of wrapping this in a useMutation? That way you don't need to keep track of isExporting etc (less chance of race conditions)
Signed-off-by: Dmitry Kalinin <[email protected]>
ActiveChooN
merged commit efd0710
into
open-edge-platform:feature/geti-inspect
2 participants
📝 Description
Added export model dialog and model action menu item
UI is blocking for now, but I'll implement download status in the status bar in my next PR
✨ 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.