-
Notifications
You must be signed in to change notification settings - Fork 22
[feature] ONNX Export #1490
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
[feature] ONNX Export #1490
Conversation
The main objective of this commit was to add a 'ONNX export' button to export trained models on the platform. Export to ONNX function is available on Dive Web & Desktop. Added a tab to list, delete and export all trained pipelines. Added the possibility to download single or multiple files from the browser on Dive web Fixed alignment issue on Jobs list on Dive web
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.
Feel free to push back on any requests I made in my comments. It's been a while since I looked at the desktop code so I may have misinterpreted things.
Within the code the only thing I have a question on is the working directory for the desktop exporting and if we should create one for exporting models. I understand that the model is being directly exported to the save location that user selects. A working directory and the log file would help in debugging any errors during export. I.E I had installed the onnx pipeline and went to export and got a generic error of 'Unable to export model' and the real problem was found in the terminal log being that my pipeline has no trained weights. An actual job log in the job history would make it so I could debug it.
This other request/suggestion is a usability concern with model deletion on the web version. Currently the trained pipeline models you have access to are any that are shared with you. So not just the ones that are in your local trained folder but if a user shared their pipelines you could see and execute them as well. This works for running pipelines but if you were also given write access to the pipeline folder you could delete another user's pipelines. That by itself is fine because before that would require a user to go into someone else folder and delete a file. The new higher level trained models tab is great but it doesn't indicate who the model is from. As admins on viame.kitware.com Matt and I actually would see everyone's pipelines by default because we have access to it. I want to make sure we don't accidently delete someones pipelines as well as users who have pipelines shared (+write access) with them don't accidently delete a pipeline too. In server/dive_server/crud_rpc.py
there is a function _load_dynamic_pipelines
we may want to include in the pipeline object an optional userId or userName derived from the userId that indicates who a pipeline is from and then in the new Models Tab have that as an additional column and possibly make the data table sortable by user.
client/platform/desktop/frontend/components/MultiTrainingMenu.vue
Outdated
Show resolved
Hide resolved
client/platform/desktop/frontend/components/MultiTrainingMenu.vue
Outdated
Show resolved
Hide resolved
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.
One small request about the error message being displayed in the Desktop Version.
The only other thing may be a discussion about what happens once the user exports on the desktop and it fails inside of the VIAME pipeline. We now have the job that runs but we don't indicate to the user that the job is running. I.E. If the onnx pipeline fails the user doesn't know anything about it until they go to the jobs panel. For simplicity, it may just be a popup message indicating that the pipeline is being exported and the status can be checked in the jobs panel. Maybe the popup even links to the jobs panel directly.
A more complicated version would be to poll the job and display the status in the 'Trained Models' v-data-table as either being success/error and linking directly to the job for review. I don't think this level is required for this PR, but it could be a future Idea.
client/platform/desktop/frontend/components/MultiTrainingMenu.vue
Outdated
Show resolved
Hide resolved
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.
Looks great!!
Thanks for tolerating my comments and change requests.
Thank you for your review :) |
The main objective of this PR was to add a 'ONNX export' button to export trained models on the platform.
Changes:
Web:

Desktop:

On Desktop, you can directly select the location where you want to save the ONNX model.
On Web, the exported model will appear in the original model directory. You can click on the eye in the Job list to go directly there.