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

New worker job: Find Largest Segment ID #6415

Merged
merged 17 commits into from
Nov 1, 2022

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Aug 19, 2022

wk side for https://github.com/scalableminds/webknossos-worker/pull/114

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Edit a dataset. Should look like this (if the largest segment id is empty):
    image

  • Click the Detect button
    image
    image

  • Should show a toast:
    image

  • After reloading:
    image
    (the message would typically be something like Updated largest segment id from 1 to 1470623.)

It would be even better if the largest segment id was automatically updated in the form, but for long running jobs this wouldn't really matter (as one can close the tab) and the interface of the API doesn't really lend itself to that currently.

Issues:


(Please delete unneeded items, merge only when none are left open)

@fm3 fm3 self-assigned this Aug 19, 2022
@fm3
Copy link
Member Author

fm3 commented Sep 29, 2022

@philippotto I adapted the job list view so that it displays the returned result message in the actions column. I’m aware that it does not really fit the column title but I don’t think creating a new column is worth it here. What do you think? Also, where do you think would be a good place for a button to trigger this job? Maybe you could have a look at the front-end part here?

…till hardcoded and incomplete, as the user is not properly informed)
@philippotto
Copy link
Member

I added some prototypical code to trigger the job (mainly so that I could test the worker). however, for a proper integration, I'd like to wait until #6485 is merged.

@philippotto philippotto marked this pull request as ready for review October 26, 2022 12:13
@philippotto
Copy link
Member

cc @fm3 the frontend is ready for review. you might want to request a review for the backend, too :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Great abstraction and UX as always! :)

I did not test this PR, mainly because I don't have a running worker setup locally, atm.

frontend/javascripts/admin/job/job_list_view.tsx Outdated Show resolved Hide resolved
frontend/javascripts/admin/job/job_hooks.ts Outdated Show resolved Hide resolved
frontend/javascripts/admin/job/job_hooks.ts Show resolved Hide resolved
@philippotto philippotto enabled auto-merge (squash) November 1, 2022 09:15
@daniel-wer
Copy link
Member

Does the backend still need a review @fm3? If not, feel free to approve (auto-merge is enabled).

@fm3
Copy link
Member Author

fm3 commented Nov 1, 2022

I can’t approve my own PR but I’d say if philipp tested that the backend passes the parameters through so that the job runs as expected, the backend does not need further review :) :shipit:

@fm3
Copy link
Member Author

fm3 commented Nov 1, 2022

I did not look at the front-end code in deatil, but one question just to make sure: is this disabled if jobs are not enabled for the instance and the dataset? (I’m asking with #6582 in mind)

@philippotto philippotto merged commit 2937be0 into master Nov 1, 2022
@philippotto philippotto deleted the find-largest-segment-id-job branch November 1, 2022 10:14
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.

3 participants