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.

Sign up for GitHub

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

Jump to bottom

Add command for asset metadata re-extraction #1545

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Add command for asset metadata re-extraction #1545

merged 2 commits into from
Oct 16, 2024

Conversation

jjnesbitt
Copy link
Member

Closes #1450
Closes #1118

@yarikoptic
Copy link
Member

What's your plan @AlmightyYakob here - wasn't it close to completion?

@jjnesbitt
Copy link
Member Author

What's your plan @AlmightyYakob here - wasn't it close to completion?

I unfortunately fell behind on this, as other things took priority. There are still logistical issues to work out on this PR, and at the moment my current priority is the embargo re-design. After that, I could take another pass at this.

@yarikoptic
Copy link
Member

this might be relevant in the scope/to address

my current priority is the embargo re-design. After that, I could take another pass at this.

embargo is a curse! ;) we should see how to move this from the stalled point. attn @waxlamp

@yarikoptic
Copy link
Member

in principle even

is related here since we might want to redo extraction upon dandischema upgrade.

@jjnesbitt do you have plans to get back to this PR?

@jjnesbitt jjnesbitt force-pushed the asset-metadata-extraction branch 6 times, most recently from 639b8bc to d5b9668 Compare September 24, 2024 18:19
@jjnesbitt jjnesbitt marked this pull request as ready for review September 24, 2024 18:21
@jjnesbitt
Copy link
Member Author

I believe this is ready to go. @jwodder or @yarikoptic can you verify that the nwb2asset function is being used correctly? As far as I'm aware from local testing it seems to be working correctly, but I don't exactly have a very good example asset to test the before and after. If you have a good example asset for this, that would also be helpful.

@jjnesbitt jjnesbitt mentioned this pull request Oct 3, 2024
Open
@jjnesbitt jjnesbitt force-pushed the asset-metadata-extraction branch 2 times, most recently from 755904c to 295857a Compare October 3, 2024 14:53
@yarikoptic
Copy link
Member

Looks good to me. I would have tried on some asset on staging and checked how/if metadata got changed.

@waxlamp
Copy link
Member

waxlamp commented Oct 9, 2024

Looks good to me. I would have tried on some asset on staging and checked how/if metadata got changed.

Could you point out a good asset on staging for this kind of test?

@yarikoptic
Copy link
Member

sorry, I don't have any specific one in mind. I could only suggest some random sensible one, e.g. what about this one
https://api-staging.dandiarchive.org/api/dandisets/200560/versions/draft/assets/ad5d6b54-9974-4559-b400-88b406be983d/
from
https://gui-staging.dandiarchive.org/dandiset/200560/draft/files?location=for_jeremy&page=1
? it is quite large so would also ensure that online access works fine

mvandenburgh
mvandenburgh requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

I can't speak to the NWB/dandi CLI code correctness, but the rest of the logic looks fine to me. Just some suggestions to use .iterator() when iterating over querysets when possible.

dandiapi/api/management/commands/extract_metadata.py Outdated Show resolved Hide resolved
dandiapi/api/management/commands/extract_metadata.py Outdated Show resolved Hide resolved
dandiapi/api/management/commands/extract_metadata.py Outdated Show resolved Hide resolved
mvandenburgh
mvandenburgh approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

If you rebase, the playwright tests should pass (see #2042)

@jjnesbitt jjnesbitt merged commit 6d91584 into master Oct 16, 2024
8 of 9 checks passed
@jjnesbitt jjnesbitt deleted the asset-metadata-extraction branch October 16, 2024 15:43
@dandibot
Copy link
Member

🚀 PR was released in v0.3.106 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yarikoptic yarikoptic yarikoptic left review comments

@mvandenburgh mvandenburgh mvandenburgh approved these changes

Assignees

@jjnesbitt jjnesbitt

Labels
released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

updating metadata on all draft assets Add a mechanism to recompute asset metadata for uploaded assets
5 participants
@jjnesbitt @yarikoptic @waxlamp @dandibot @mvandenburgh