Skip to content

Client/pipeline ux#558

Merged
subdavis merged 8 commits into
masterfrom
client/pipeline-ux
Feb 5, 2021
Merged

Client/pipeline ux#558
subdavis merged 8 commits into
masterfrom
client/pipeline-ux

Conversation

@subdavis
Copy link
Copy Markdown
Contributor

@subdavis subdavis commented Feb 4, 2021

Fixes #551

I certainly agree that more could be done, but this is the most I'm able to do at the moment. I think it's a low-cost improvement.

I agree that adding queueing info would be nice, but it would have to change the common desktop/web apispec to get that info.

Opted for a simpler change under the constraints we have already.

Screenshot from 2021-02-04 09-15-04
Screenshot from 2021-02-04 09-14-49

@subdavis subdavis requested a review from BryonLewis February 4, 2021 14:25
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well to serve the initial purpose of indicating that the job has actually started.
I think next step would be to global.state the running pipeline list from the Jobs Tab Icon there and use that in conjunction with your dialog as an intermediate before actually running the pipeline. Where it would indicate to the user that a pipeline is being run by you on the current dataset already and give them the option to continue and realize it will be queued or to cancel.
Other than that just two minor things:
The correction on the jobs link.
If you want to adjust the styling on the v-menu to fit it inside of the current viewport.

<span>{{ item.statusText.replace('Inactive', 'In Queue') }}</span>
<v-btn
x-small
:href="`/girder/#jobs/${item._id}`"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be /#job not /#jobs/
I believe /#jobs would take you to the main area but it needs to be /#job/${id} to go to the individual job data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Could you make that change in what you push?

If not, I'll do it.

Thank you!

Comment thread client/viame-web-common/components/RunPipelineMenu.vue
@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Feb 4, 2021

I think maybe we should also add some info on the job page about how jobs work, what the queue is, and re-iterate the warning about weekly deployments

Just like, definitions and some "this is a shared server, you might have to wait your turn" type info.

@BryonLewis
Copy link
Copy Markdown
Collaborator

Just like, definitions and some "this is a shared server, you might have to wait your turn" type info.
Couldn't hurt to add that in.
If you want to I would say also swap the 'Manage' button to just a direct 'Cancel' button. I don't mind them going to Girder, but users getting curious inside of girder could cause more problems than it's worth. (Them going to their own folder, trying to upload stuff outside of your system). I don't what else they would be doing inside of the girder jobs panel, is it to their benefit to look at the logs?

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Feb 4, 2021

Manage' button to just a direct 'Cancel' button.

I considered it, I was sort of on the fence. but the manage button was easy and required very little code.

I don't feel strongly, but I don't think we have very many people who need to cancel stuff. Especially since we now have better UI for creation to prevent duplicates.

Maybe if we start seeing users actually use the cancel button we can move it into the job list?

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Feb 4, 2021

On another note, I was thinking about putting a big "Danger: don't mess with stuff if you don't know what you're doing" warning in girder client through the backbone plugin system.

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Feb 4, 2021

These opinions reflect a larger "pro-girder" shift in my thinking about our customers and VIAME web.

I definitely have biases, and if you want to talk about that or feel differently, I'd like to talk about it.

@BryonLewis
Copy link
Copy Markdown
Collaborator

BryonLewis commented Feb 4, 2021

These opinions reflect a larger "pro-girder" shift in my thinking about our customers and VIAME web.

I definitely have biases, and if you want to talk about that or feel differently, I'd like to talk about it.

I think some of this warrants a discussion if we are going to transition to girder-next at some point. With what is being describe as the more DIY system that doesn't have some of the built in stuff it may be beneficial to consider other alternatives if they slot in well. (TS Node, we have to do half of it for electron anyways and datatypes can remain consistent. There are drawbacks to consider as well like some of the file processing we may wan't to offload from Node).

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Feb 4, 2021

Added some backend changes to prevent launching jobs on datasets with pre-existing jobs.

Also added new provenance fields on the job that we may want to use in the future.

@subdavis subdavis requested a review from BryonLewis February 4, 2021 22:27
@BryonLewis
Copy link
Copy Markdown
Collaborator

One question and one sidenote:

Question - I was thinking more a warning that one is running instead of a flat out refusal to queue pipelines. We may want to check with Matt but are there instances in which you would want to intentionally chain pipelines? Like if we ran the utility full frame detection generator and then wanted to pipe that into a full frame classifier afterwards. Are we going to have a different interface for pipeline 'macros' or is this such a niche case it isn't needed.

Side-Note: I didn't see your job(s) reference for the manage button until I had already did the PR of my UI change so that needs to be swapped as well.

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Feb 5, 2021

Like if we ran the utility full frame detection generator and then wanted to pipe that into a full frame classifier afterwards. Are we going to have a different interface for pipeline 'macros' or is this such a niche case it isn't needed.

I do not think such a case should exist, even if it does now. The utility pipelines need to be implemented client side (aside), and if a pipeline in viame expects full frame boxes as a prerequisite, it should generate them if they don't exist, not throw an error that we have to deliver to the user to tell them to run some other pipeline first.

This is a user experience concern. We are already struggling to provide clear guidance on how pipelines work. Trying to communicate dependencies / relationships between pipelines doesn't sound like a good choice to me.

These are my opinions. Matt might have more input.

Are we going to have a different interface for pipeline 'macros' or is this such a niche case it isn't needed.

I hope that we are not, but this is an interesting question we haven't discussed. We should bring it up next week.

I didn't see your job(s) reference for the manage button until I had already did the PR of my UI change so that needs to be swapped as well.

Thanks, I'll fix that.

Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the issue currently with both the pipeline list and running duplicates and makes it easier to see job output. If we need cascading pipelines I think that is a bigger future discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] [UX] Better ux for job creation

2 participants