-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add Render Animation Worker Job #7348
Conversation
…s into render-animation
@@ -684,6 +690,18 @@ class TracingActionsView extends React.PureComponent<Props, State> { | |||
} | |||
|
|||
menuItems.push(screenshotMenuItem); | |||
|
|||
if (activeUser?.isSuperUser) { |
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 is by design. Let's do a soft launch and try some larger datasets on the server first before releasing this publicly.
@philippotto I think this is ready for review. Feel free to only review the WK side. In case you setup up the worker too, perhaps you can do a rough review for that as well. Thanks. I will create a page for the WK documentation later this week in a separate PR. Let's try to get this merged and soft launched for super users so that we can iron out any issue with the worker deployment and test performance in the real world. |
I’d say the backend changes are straightforward enough that they don’t require a specialized review. @philippotto maybe you could quickly read through them as well while checking the front-end |
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.
awesome stuff 👍 only left a couple of nitpicks.
also, one thing UX wise: I'd put the word "video" somewhere to clarify that the output medium is a video file. otherwise, it might be unclear to the user what they will get (a gif? a blender file? an upload to youtube?).
frontend/javascripts/oxalis/view/action-bar/create_animation_modal.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/create_animation_modal.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/create_animation_modal.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/create_animation_modal.tsx
Outdated
Show resolved
Hide resolved
let meshFileName = "meshfile.hdf5"; | ||
let segmentationLayerName = "segmentatation"; |
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's up with these? why not initialize these with null
and then show an error in case no mesh files were found later?
also, there's a typo in segmentation.
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.
Mhm, the backend expects a string - hence the defaults. If any mesh is rendered is mostly controlled by the submitted segment IDs.
Showing an error is a good idea.
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.
If this is optional then I’d say the backend should also understand that. I can probably add that today.
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 rechecked the backend and worker code, and there the type is segmentationLayerName: Option[String]
and meshFileName: Option[String]
, so it should be possible to make this optional in the frontend already.
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 switched the frontend values to undefined
by default.
frontend/javascripts/oxalis/view/action-bar/create_animation_modal.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/create_animation_modal.tsx
Outdated
Show resolved
Hide resolved
I did that yesterday, too, and everything looked good to me :) |
…odal.tsx Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
…odal.tsx Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
@fm3 Did you built email notifications for this job as well? |
I did not adapt the email code, no. My understanding is that a generic job completed email will be sent? Feel free to add a specialized email though :) Or I could do it as well if you tell me what it should say |
I haven't tried the email notifications on my local dev setup (not configured). Let's start a soft launch and figure this out as well. |
Sounds fair :) Then I guess the only remaining blockers are the frontend code review comments by philipp |
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.
Good stuff 👍
Follow Up Issue: #3350 |
This PR adds the frontend and backend feature for starting a animation job of the current annotation / dataset.
URL of deployed dev instance (used for testing):
Steps to test:
Issues: