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

Fix uploading zipped tiff (should set needsConversion=true) #7856

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jun 4, 2024

Fixes two bugs in dataset upload

  • The frontend failed to set needsConversion=true in case of zipped data (e.g. a zip with tiffs), because the return statement exited not only the for loop but the whole function
  • The backend detected N5 data too eagerly (return value of containsMatchingFile was not used)

Steps to test:

  • yarn enable-jobs (no actual worker needed though)
  • In dataset upload view, upload a zip with tiff files
  • After the upload, a conversion job should be started
  • Upload bare tiffs, upload wkw, these other cases should still behave as expected

Issues:


@fm3 fm3 self-assigned this Jun 4, 2024
@fm3 fm3 marked this pull request as ready for review June 4, 2024 12:40
@@ -513,7 +513,7 @@ class DatasetUploadView extends React.Component<PropsWithFormAndRouter, State> {
}
// We return here since not more than 1 zip archive is supported anyway. This is guarded
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// We return here since not more than 1 zip archive is supported anyway. This is guarded
// We break here since not more than 1 zip archive is supported anyway. This is guarded

Copy link
Member

Choose a reason for hiding this comment

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

Maybe rephrase entire comment

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, done :)

@fm3 fm3 requested a review from hotzenklotz June 6, 2024 09:57
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Works 🎉

I tested all tiff formats included in: https://www.notion.so/scalableminds/Test-Datasets-c0563be9c4a4499dae4e16d9b2497cfb?pvs=4#7db7f26b5e494352a8153ab073487ab3 as well as wkw.

Just left one question :)

Comment on lines +293 to +294
firstExploredDatasource <- dataSources.headOption.toFox
dataSource = GenericDataSource[DataLayer](dataSourceId, combinedLayers, firstExploredDatasource.scale)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the advantage of this change?
To me the code semantically reads the same. Just that the testing for whether dataSources has atleast a one item is more explicit. Or am I misunderstanding something here?

Copy link
Member Author

@fm3 fm3 Jun 7, 2024

Choose a reason for hiding this comment

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

head on an empty list raises an exception, which bypasses some of our error handling. headOption.toFox creates a Fox.failure that will yield nicer error messages in our context.
I’d say head is always a codesmell in our codebase, unless it is directly wrapped in a tryo (but then headOption should always be possible instead) or we are absolutely certain that the list will not be empty.

@fm3 fm3 enabled auto-merge (squash) June 11, 2024 05:32
@fm3 fm3 merged commit 08ce2ba into master Jun 11, 2024
2 checks passed
@fm3 fm3 deleted the fix-zip-upload branch June 11, 2024 05:51
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.

uploading a zip with tiff files does not set needsConversion=true
3 participants