-
Notifications
You must be signed in to change notification settings - Fork 853
🚀 feat(inspect): Allow user to upload images, train model and display training progress #2984
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
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
MarkRedeman
left a comment
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 good, thanks @dwesolow !
I have some minor comments but feel free to handle them in follow up PRs
| const mediaItemsToRender = | ||
| mediaItems.length >= REQUIRED_NUMBER_OF_NORMAL_IMAGES_TO_TRIGGER_TRAINING | ||
| ? mediaItems | ||
| : Array.from({ length: REQUIRED_NUMBER_OF_NORMAL_IMAGES_TO_TRIGGER_TRAINING }).map((_, index) => | ||
| index <= mediaItems.length - 1 ? mediaItems[index] : undefined | ||
| ); |
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.
np: I think this is a bit more readable, (it makes it easier to see that we render the media items by default, and potentially add some undefined placeholders)
| const mediaItemsToRender = | |
| mediaItems.length >= REQUIRED_NUMBER_OF_NORMAL_IMAGES_TO_TRIGGER_TRAINING | |
| ? mediaItems | |
| : Array.from({ length: REQUIRED_NUMBER_OF_NORMAL_IMAGES_TO_TRIGGER_TRAINING }).map((_, index) => | |
| index <= mediaItems.length - 1 ? mediaItems[index] : undefined | |
| ); | |
| const mediaItemsToRender = [ | |
| ...mediaItems, | |
| ...Array.from({ length: Math.max(0, REQUIRED_NUMBER_OF_NORMAL_IMAGES_TO_TRIGGER_TRAINING - mediaItems.length) }) | |
| ] |
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.
Nice, will add it, thanks!
| { | ||
| id: 'padim', | ||
| name: 'PADIM', | ||
| }, |
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.
@ashwinvaidya17 can you comment on what other models should be available? The id that is used here will be used in the model training service.
|
|
||
| return ( | ||
| <InlineAlert variant='negative'> | ||
| <Heading>Training canceled</Heading> |
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'd maybe also add a if (jobProgres.status === 'cancelled') here instead of relying that type inference guarantees that the status must be cancelled.
Having the explicit if statement here makes it a bit easier to read when reviewing outside of an IDE (I don't have to make sure that in the if statements above we handled all except the cancelled status)
| // TODO: Investigate how to handle that case (local storage, poll for job status) after refreshing a page. | ||
| if (startTrainingMutation.data !== undefined) { |
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.
Can we use the '/api/jobs' endpoint for this? Perhaps we can make it so that if the jobs endpoint returns 1 or more jobs for this project, then we render all of those jobs' progresses.
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.
Yeah I was wondering implementing this but I wasn't sure but if u also agree then I'll implement that ;).
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.
Let's go with this on the short term.
Long term we have to see how long training takes and how frequently we expect new models to be trained (and if they are trained in bulk), if they are then I think a job management panel like we have in Geti would be a good approach.
| const captureImages = (files: FileList | null) => { | ||
| if (files === null) return; | ||
|
|
||
| Array.from(files).forEach((file) => captureImage(file)); |
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.
It might be better to change this function to an async function and await each mutation. Otherwise if the user selects 1000 images then we would immediately try to upload all of them.
Though given that we will likely remove this feature once the source pipeline is integrated I guess it won't be worth it to optimize this?
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.
Yeah, I wanted to keep it really simple since it's a temporary feature.
… training progress (#2984) * feat: Allow user to upload images * feat: List uploaded images * chore: Extract components to separate files and add ready to train and training progress * chore: comment thumbnail url generation
… training progress (open-edge-platform#2984) * feat: Allow user to upload images * feat: List uploaded images * chore: Extract components to separate files and add ready to train and training progress * chore: comment thumbnail url generation
… training progress (#2984) * feat: Allow user to upload images * feat: List uploaded images * chore: Extract components to separate files and add ready to train and training progress * chore: comment thumbnail url generation
📝 Description
This PR introduces a few features:
This is just starting point, we will improve that once we know that training is stable.
✨ 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.