-
Notifications
You must be signed in to change notification settings - Fork 24
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
Hybrid task with nml #4198
Hybrid task with nml #4198
Conversation
Frontend ToDo:
|
front-end is done! |
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.
Code LGTM 👍
Things I noticed:
- (If I create a hybrid task type, then a task with that task type and open that task, I'm only allowed to do volume actions (it's also displayed as type
volume
in the info tab). Did I miss a step here to get a proper hybrid task?) - (In the
dashboard/tasks
view, only avolume
badge is shown for hybrid tasks, but bothskeleton
andvolume
should be shown.) - I see - Both of the above points are solved if the base annotation is a hybrid tracing instead of a volume tracing - that's probably fair for now.
- If I supply a base annotation that is a hybrid tracing to a task with a volume task type, the resulting task is a hybrid task. So basically the base annotation tracing type overwrites the task type tracing type. That is, because the frontend decides what type of tracing there is (and what type of actions the user can take), based on whether there is a skeleton and/or volume tracing present. I would say that is alright for now, but we should probably fix that at some point, it seems un-intuitive.
Thank you for the thorough testing @daniel-wer! I didn't notice that the base annotation type takes precedence. However, I'd suggest to fix this already in this PR, since I think it's a likely scenario that hybrid tasks should be created and the base annotation is only a skeleton. The other direction ("create skeleton task and base is hybrid") might be less likely, but I think that should be even easier to fix, @youri-k? So, I guess these two to dos remain for the back end:
@youri-k Do you agree with this plan? |
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.
While I agree that it is quite dubious to fill the optional tracingid fields in the BaseAnnotation objects, I also see that the nearby alternatives are more tedious. Please add a changelog entry before merging
I’d possibly add a warning to the user that this is a slow operation, and should not be done with too many tasks at a time, something like this. What do you think @philippotto ? Apart from that, I guess we can merge and deploy. |
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
[ ] Updated migration guide if applicable[ ] Updated documentation if applicable[ ] Adapted wk-connect if datastore API changes[ ] Needs datastore update after deployment