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

Tiff Export Job #5195

Merged
merged 18 commits into from
Mar 8, 2021
Merged

Tiff Export Job #5195

merged 18 commits into from
Mar 8, 2021

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Feb 22, 2021

Lets the user download a zipfile with tiff files, exported from wkw on disk.

  • New api routes api/jobs/run/tiffExport/:organizationName/:dataSetName/:layerName/:bbox (with bbox being in format x,y,z,w,h,d)
  • api/jobs/downloadExport/:organizationName/:exportFileName (jobs json may now include exportFileName, I already adapted api flow types)

Todo:

Steps to test:

  • Set up https://github.com/scalableminds/webknossos-worker/pull/5 (Will be merged into master there once this is approved)
  • set features.jobsEnabled = true in application.conf
  • Create annotation, create user bounding box
  • In user bounding box list, you should see download icons, opens modal
  • select layer, see that job is running in jobs page
  • when complete, download zip, should contain selected data range

Issues:


@fm3
Copy link
Member Author

fm3 commented Feb 22, 2021

@philippotto Would you be interested in discussing how the front-end for this could look like? :)

@fm3 fm3 changed the title [WIP] Tiff Export Job Tiff Export Job Feb 23, 2021
@fm3 fm3 requested review from philippotto and youri-k February 23, 2021 11:55
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.

Code LGTM :shipit:

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.

Awesome! Looks quite cool. I'll give it a test now :)

@philippotto
Copy link
Member

I just tested the feature and after some hiccups with my wk-worker-setup it worked 🎉 It would be very cool if the errors would bubble up into the jobs-page. However, a proper error in the worker-stdout would already go a long way (it just said process.CalledProcessError: Command '['python', '-m', 'wkcuber.export_wkw_as_tiff', '--layer_name', 'color', '--bbox', '500,500,500,100,100,100', '--source_path', '/data/sample_organization/ROI2017_wkw', '--destination_path', '/data/sample_organization/.converting/20 21-02-55_15-22__ROI2017_wkw__color__h9LBAgVIiNeb/tif']' returned non-zero exit status 1). First, the file mount was wrong and then I had a permission problem. Obviously, that was a setup mistake on my part, but maybe we can improve the error handling a bit here.

During testing I noticed two things:

  • there doesn't seem to be a limit on the bounding box size. shouldn't we disable the export button if the bbox is larger than X voxels?
  • the name of the downloaded zip archive could look a bit nicer. for me it was 2021-02-55_15-27__ROI2017_wkw__color__AEVEq1j1JdfR.zip. maybe we could drop the random postfix?
  • the layer name doesn't appear in the job description. if one exports multiple layer, one has to click "download" to see which one is which:
    image

@fm3
Copy link
Member Author

fm3 commented Feb 25, 2021

Thanks for having a look!

However, a proper error in the worker-stdout would already go a long way

I could not yet figure out how to do this, but I’ll look again. I agree that it would be very valuable.

there doesn't seem to be a limit on the bounding box size. shouldn't we disable the export button if the bbox is larger than X voxels?

Yes, good point! What do you think would be a sensible default for this limit? (Also cc @normanrz )

maybe we could drop the random [filename] postfix?

I added it to avoid name collisions. Do you think it is safe to remove it? I guess we can add seconds to the timestamp to reduce the risk.

@philippotto
Copy link
Member

However, a proper error in the worker-stdout would already go a long way

I could not yet figure out how to do this, but I’ll look again. I agree that it would be very valuable.

Since my stdout showed a thrown process.CalledProcessError, it's probably a good approach to catch that (and potentially re-raise? don't know how the protocol with the worker looks in error cases). This post also shows how to extract the actual output. I hope this works? https://stackoverflow.com/a/47185863

there doesn't seem to be a limit on the bounding box size. shouldn't we disable the export button if the bbox is larger than X voxels?

Yes, good point! What do you think would be a sensible default for this limit? (Also cc @normanrz )

How about 1024**3 Vx ~ 1 GB?

maybe we could drop the random [filename] postfix?

I added it to avoid name collisions. Do you think it is safe to remove it? I guess we can add seconds to the timestamp to reduce the risk.

You mean collisions on the server's file system, right? Ideally, the postfix would stay there, but the default name when downloading wouldn't include this. I think, this is possible by setting a HTTP header à la: header('Content-Disposition: attachment; filename="2021-02-55_15-27__ROI2017_wkw__color.zip"');

@normanrz
Copy link
Member

Yes, good point! What do you think would be a sensible default for this limit? (Also cc @normanrz )

Initially, I was thinking of (1024vx)**3

@normanrz
Copy link
Member

Btw is it downloading all layers (including segmentation)?

@philippotto
Copy link
Member

Btw is it downloading all layers (including segmentation)?

the user can select the layers they want to download. downloading volume is not supported yet (since that part has to go via the tracingstore). I think, @fm3 suggested to do this in a separate PR.

@fm3
Copy link
Member Author

fm3 commented Feb 26, 2021

Note to self: Todo:

  • restrict bbox size (both volume and edge length)
  • fix flow + eslint
  • make sure that cubing jobs also still work

@fm3
Copy link
Member Author

fm3 commented Mar 1, 2021

@philippotto Thanks for your feedback! I think I solved all raised issues, could you have another look?

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.

Looks great! :) See my few comments before merging.

@fm3 fm3 merged commit d2ff03d into master Mar 8, 2021
@fm3 fm3 deleted the tiff-export branch March 8, 2021 06:59
@fm3 fm3 mentioned this pull request Mar 8, 2021
1 task
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.

Export Bounding Box to Tiff
4 participants