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

Disable summary stats temporarily #386

Merged
merged 4 commits into from
Jul 2, 2021
Merged

Disable summary stats temporarily #386

merged 4 commits into from
Jul 2, 2021

Conversation

waxlamp
Copy link
Member

@waxlamp waxlamp commented Jul 1, 2021

This is a workaround to speed up the bulk updating of asset metadata in dandisets with a large number of assets.

The way the code is currently written, each asset metadata update results in an O(N) computation across all the assets, making an update of all asset metadata an O(N^2) operation in total. This workaround drops that to O(N) at the cost of not updating the summary stats.

@AlmightyYakob and @mvandenburgh helped me review this as I wrote it, so I'm formally self-reviewing and merging to master so that it appears on staging; if it behaves well there, I will promote to production so that @satra is able to speed up his process.

attn: @dchiquito

Closes #384.

This is meant as a temporary workaround to prevent costly summary
generation when we are in the middle of making large batches of asset
metadata updates.
@yarikoptic
Copy link
Member

circle-ci,

  Created wheel for dandiapi: filename=dandiapi-test_2_2.gb980e1c-py3-none-any.whl size=74448 sha256=4e65cb50bcde7fe499e5b12c1e6c2e463ccce005695ee8559b221bbd3b13698f
  Stored in directory: /home/circleci/.cache/pip/wheels/a3/5a/8d/d11b591aeba6a2aa39eafeac55c46e733faddfa95668208e50
  WARNING: Built wheel for dandiapi is invalid: Metadata 1.2 mandates PEP 440 version, but 'test-2-2.gb980e1c' is not
Failed to build dandiapi
ERROR: Could not build wheels for dandiapi which use PEP 517 and cannot be installed directly
WARNING: You are using pip version 21.1.2; however, version 21.1.3 is available.

reminds me about the issue we fixed all around, and boiling down to shallow clones (ref also #222 (comment)), some reference from similar expeditions in datalad: datalad/datalad#5669 (comment) . I didn't check circle-ci history in how come it didn't come up before (filed con/tinuous#124 just in case)...

dandi-cli: expected given the changes:

E               ValueError: Dandiset 000029 is invalid: 1 validation error for PublishedDandiset
E               assetsSummary
E                 A Dandiset containing no files or zero bytes is not publishable (type=value_error)

and I guess would be ok to close eyes at for the duration of the workaround

@dchiquito dchiquito self-requested a review July 2, 2021 13:32
@waxlamp waxlamp merged commit 93faf5f into master Jul 2, 2021
@waxlamp waxlamp deleted the workaround-no-summary branch July 2, 2021 17:11
yarikoptic added a commit that referenced this pull request Jul 3, 2021
This reverts commit 93faf5f, reversing
changes made to da32e0e.
yarikoptic added a commit that referenced this pull request Jul 5, 2021
Revert "Merge pull request #386 from dandi/workaround-no-summary"
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.

performance of PUT request
3 participants