-
Notifications
You must be signed in to change notification settings - Fork 1.7k
audio: add generic data support #3514
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
Merged
Merged
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
5f43a27
audio: remove labels from UI
wchargin 54c2da2
uploader: inline graph filtering from `dataclass_compat`
wchargin 2cde07d
backend: move compat transforms to event file loading
wchargin f339182
dataclass_compat: track initial tag metadata
wchargin 61ab333
[update patch]
wchargin 45b02e7
[update diffbase]
wchargin 76843be
[update diffbase]
wchargin a12f351
[update patch]
wchargin a5e8fa5
audio: add generic data support
wchargin 8a4247c
[update patch]
wchargin aac18b4
[bump ci]
wchargin 8ef2886
[update diffbase]
wchargin f159fa5
[update patch]
wchargin 6d11b7c
[update diffbase]
wchargin 6c2d8ae
[update diffbase]
wchargin 7ea1d0c
[update diffbase]
wchargin ab52a51
uploader_test: check logical equality of protos
wchargin d17e9f9
[update diffbase]
wchargin 84fd685
[update diffbase]
wchargin ff15dc7
[update diffbase]
wchargin a0edd31
[update diffbase]
wchargin 1a04730
[update diffbase]
wchargin 94a974c
[update diffbase]
wchargin 2ac4935
[update patch]
wchargin 7c33f3e
[update diffbase]
wchargin a9adf93
[update diffbase]
wchargin 8f469cc
[update patch]
wchargin ac58c6d
[update diffbase]
wchargin 61cc397
[update diffbase]
wchargin 524d0bc
[update patch]
wchargin 94e1afe
[update diffbase]
wchargin 748938a
[update diffbase]
wchargin 82310b4
[update diffbase]
wchargin d553605
[update patch]
wchargin 83b3404
[update diffbase]
wchargin 254eb40
[update diffbase]
wchargin 3d1d73f
[update patch]
wchargin 0a6d49f
[update diffbase]
wchargin c1da800
[update patch]
wchargin 12af0db
[update diffbase]
wchargin 800a3b1
[update patch]
wchargin 2b095ce
[update diffbase]
wchargin fd201f6
[update patch]
wchargin ba64453
[update diffbase]
wchargin 1cc2861
[update diffbase]
wchargin 28a8148
[update patch]
wchargin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this possible issue! Can we add a test that exercises this to ensure that we refuse to return audio as, say,
text/javascript?I think the Werkzeug client
get()API takes acontent_typeparameter so this should be straightforward.In the longer term it would be nice if we didn't have to rely on the client behavior for this. Perhaps we should add a per-blob
content_typeonto the blob data model? We would have to go around adding it to the various data providers and being sure to set it correctly on upload, but it seems worth doing to properly handle cases like this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure: done, I think, though I didn’t understand what you meant about the
Werkzeug client API taking a
content_typeparameter. (Isn’t that forthe content type of a POST request body?)
Perhaps. Presumably the source of truth would be a new field on the
SummaryMetadataproto (string content_type = 5;), required to behomogeneous within a time series across all steps and indices, and
defaulting to
application/octet-streamif empty for backwardcompatibility? I’ll keep it in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was confused and misread this and somehow thought we were getting the content type from a header that the browser was populating (even though yes, that isn't a thing for GET requests) rather than the query parameter. Never mind :P
Re: per-blob content types, yeah, I guess it could have a field in SummaryMetadata, although it's perhaps a little odd to have a field that is returned per-value stored in the per-tag SummaryMetadata (even if for now we don't anticipate allowing it to vary per-value).