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

When downloading with skipVolumeData, keep volume tag #6566

Merged
merged 4 commits into from
Nov 7, 2022

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Oct 19, 2022

URL of deployed dev instance (used for testing):

Steps to test:

  • Download a volume annotation with and without the include-volume-data checkbox active
  • The volume tag and its children (segments) should always be included
  • in the skip-volume-data case, the location attribute should be omitted, and a comment added instead.

@fm3 fm3 requested a review from jstriebel October 19, 2022 08:17
@fm3 fm3 self-assigned this Oct 19, 2022
@fm3
Copy link
Member Author

fm3 commented Oct 19, 2022

Looks like this does break the download with skipVolumeData for libs versions 0.10.18 and 0.10.19. Do you think we should accept that and encourage users to upgrade? Or build a backwards compatible version of this? I’m hesitant to just make the location emptystring, maybe you have other ideas?

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

The code itself LGTM 👍

Looks like this does break the download with skipVolumeData for libs versions 0.10.18 and 0.10.19. Do you think we should accept that and encourage users to upgrade? Or build a backwards compatible version of this? I’m hesitant to just make the location emptystring, maybe you have other ideas?

As a first measure we should release a wk-libs version that is compatible with the change, I prepared a PR for that (scalableminds/webknossos-libs#814). Then I think it's ok to merge this, since it only affects new downloads, and skip_volume_data must be set, which was just introduced three weeks ago with version 0.10.18 here.

@fm3
Copy link
Member Author

fm3 commented Oct 22, 2022

Thanks for having a look and thanks for pushing the change in the python client!

Do you think we should wait for some days before deploying this, so that more people would switch from 0.10.18 to 0.10.20? Or maybe it’s a rare enough case that it doesn’t make sense to wait?

@jstriebel
Copy link
Contributor

Do you think we should wait for some days before deploying this, so that more people would switch from 0.10.18 to 0.10.20? Or maybe it’s a rare enough case that it doesn’t make sense to wait?

Let's just wait a couple of days, as this change is not pressing IMO. But in general I think that it should be quite rare, still.

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

Enough waiting IMO 🚀

@fm3 fm3 merged commit 45e97b6 into master Nov 7, 2022
@fm3 fm3 deleted the volume-skip-data-include-tag branch November 7, 2022 07:48
hotzenklotz added a commit that referenced this pull request Nov 8, 2022
…sages

* 'master' of github.com:scalableminds/webknossos:
  Remove superfluous datastoreName param for convertToWkw job (#6606)
  When downloading with skipVolumeData, keep volume tag (#6566)
  Fix getNullBucket crash and remove dead code (#6603)
  Hotfix Dataset View Mode
hotzenklotz added a commit that referenced this pull request Nov 8, 2022
…cing

* 'master' of github.com:scalableminds/webknossos:
  Remove superfluous datastoreName param for convertToWkw job (#6606)
  When downloading with skipVolumeData, keep volume tag (#6566)
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.

2 participants