Skip to content
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 manual pass state for long-running jobs #5530

Merged
merged 12 commits into from
Jun 10, 2021
Merged

Add manual pass state for long-running jobs #5530

merged 12 commits into from
Jun 10, 2021

Conversation

fm3
Copy link
Member

@fm3 fm3 commented May 31, 2021

Shows convert_to_wkw jobs that failed as “Manual“ (meaning an admin will try to repair this job), unless an admin has alreadyset the job’s manualState to SUCCESS or FAILURE in the database.

Steps to test:

  • set up worker, jobsEnabled
  • upload datasets that work and that don’t work
  • run other jobs as well
  • in postgres, set a job to manually repaired or failure (i.e. wontfix)
  • that state should be shown then, links should become available in success case

Note:

I am unsure about the wording (e.g. the displayed name “Manual” for that new state), and the tooltips. Feedback welcome :)

Issues:


@fm3 fm3 changed the title [WIP] Add manual pass state for long-running jobs Add manual pass state for long-running jobs May 31, 2021
@fm3 fm3 requested review from philippotto and youri-k May 31, 2021 11:42
@fm3 fm3 marked this pull request as ready for review May 31, 2021 11:42
@fm3
Copy link
Member Author

fm3 commented Jun 1, 2021

@youri-k I would like to add a line to the evolution setting the manualState to FAILURE for all existing jobs where the celery state is also FAILURE (because we don’t want to repair all old jobs). Since you are more familiar with json in SQL, could you give me a pointer there? No hurry.

@philippotto I incorporated your feedback :) Another question: Do you think we should make this feature opt-in? (e.g. add a manualPassJobs array to the features block of the config?) Currently I think we have only one instance with jobsEnabled, but that may change

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Front-end looks great! However, I added some comments regarding the typing which we should harden a bit to make the job handling more robust. The typing was already not optimal before this PR, but I think we should use the chance now to improve it :)

frontend/javascripts/admin/job/job_list_view.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
frontend/javascripts/admin/admin_rest_api.js Outdated Show resolved Hide resolved
frontend/javascripts/types/api_flow_types.js Show resolved Hide resolved
@fm3
Copy link
Member Author

fm3 commented Jun 3, 2021

@philippotto Thanks for your feedback! :) Could you have another look?

@youri-k
Copy link
Contributor

youri-k commented Jun 3, 2021

Since you are more familiar with json in SQL, could you give me a pointer there?

I hope this line from the evolution 57 is helpful:

UPDATE webknossos.dataSet_layers dl
SET adminViewConfiguration = dS.adminViewConfiguration->'layers'->dl.name
FROM webknossos.dataSets dS
WHERE dl._dataSet = dS._id AND dS.adminViewConfiguration ? 'layers' AND dS.adminViewConfiguration->'layers' ? dl.name;

This sets the layer's adminViewConfiguration based on the entry of the layer in the general adminViewConfiguration. The WHERE clause checks whether the adminViewConfiguration contains a layer object and whether that object contains the specific layer entry.

I really like this cheat sheet, where all the operators are explained: https://devhints.io/postgresql-json

Hope this helps, but you can ping me again if something does not work

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Front-end looks top notch 🎉

Copy link
Contributor

@youri-k youri-k left a comment

Choose a reason for hiding this comment

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

Backend and SQL LGTM 🎉

@fm3 fm3 enabled auto-merge (squash) June 10, 2021 07:03
@fm3 fm3 merged commit 7969d85 into master Jun 10, 2021
@fm3 fm3 deleted the manual-pass branch June 10, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

“Manual pass” state for long-running jobs Job list view: Tooltip on “Started” state has wrong text
3 participants