data: perform downsampling in multiplexer provider #3272
Merged
Conversation
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
Summary: We add a `sampling_hints` attribute to the `TBContext` magic container, which is populated with the parsed form of the `--samples_per_plugin` flag. Existing plugins’ generic data modes are updated to read from this map instead of using hard-coded thresholds. Test Plan: This change is not actually observable as is, because the multiplexer data provider ignores its downsampling argument. But after patching in a change to make the data provider respect the downsampling argument, this change has the effect that increasing the `--samples_per_plugin` over the default (e.g., `images=20`) now properly increases the number of samples shown in generic data mode, whereas previously it had no effect. wchargin-branch: data-downsampling-flag wchargin-source: 50998be15abd790a0915458bac76091c79823f0f
Summary:
The `MultiplexerDataProvider` now respects its `downsample` parameter,
even though the backing `PluginEventMultiplexer` already performs its
own sampling. This serves two purposes:
- It enforces that clients are always specifying the `downsample`
argument, which is required.
- It enables us to test plugins’ downsampling parameters to verify
that they will behave correctly with other data providers.
Test Plan:
Unit tests included. Note that changing the `_DEFAULT_DOWNSAMPLING`
constant in (e.g.) the scalars plugin to a small number (like `5`) now
actually causes charts in the frontend to be downsampled.
wchargin-branch: data-mux-downsample
wchargin-source: 116ce4c206613e25e09fec31102b90ad80282496
wchargin
requested review from
nfelt and
stephanwlee
and removed request for
nfelt
Contributor
Author
|
(Reassigning just to spread load.) |
stephanwlee
approved these changes
Feb 21, 2020
Contributor
stephanwlee
left a comment
stephanwlee
left a comment
There was a problem hiding this comment.
It enforces that clients are always specifying the downsample
argument, which is required.
Is this saying all data_provider, in the future, will require downsample argument? Or is it only true for MultiplexerDataProvider?
|
|
||
| def test_inorder(self): | ||
| xs = list(range(10000)) | ||
| actual = data_provider._downsample(xs, k=100) |
Contributor
There was a problem hiding this comment.
No AI. Simply noting: there is non-zero chance where this passes even if it behaved more like random.sample. :\
Contributor
Author
There was a problem hiding this comment.
Yep, but that chance is 1 / (100!), which is somewhat small.
wchargin
changed the base branch from
wchargin-data-downsampling-flag
to
master
wchargin-branch: data-mux-downsample wchargin-source: c6edc8bfebc0d4c040c346e8ca9729e399512f25
wchargin
commented
Feb 21, 2020
Contributor
Author
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
Thanks for the quick reviews!
Is this saying all data_provider, in the future, will require
downsampleargument? Or is it only true forMultiplexerDataProvider?
It’s required by the data provider interface (e.g., read_scalars),
so any particular data provider implementation is permitted to not
require it, but code that is written against arbitrary data providers
must provide it.
|
|
||
| def test_inorder(self): | ||
| xs = list(range(10000)) | ||
| actual = data_provider._downsample(xs, k=100) |
Contributor
Author
There was a problem hiding this comment.
Yep, but that chance is 1 / (100!), which is somewhat small.
wchargin
added a commit
that referenced
this pull request
Mar 15, 2021
Summary: As the comment says, the histograms plugin used to manually downsample because the multiplexer data provider didn’t. As of #3272, it does, so this code is vestigial. Test Plan: Note that changing the default histogram sampling constants (in the same file) to very small numbers causes the frontend to show less data. Unit tests for the histograms plugin also cover this: removing downsampling from the multiplexer data provider causes the histogram tests to fail. wchargin-branch: histogram-rm-downsample wchargin-source: 33928fd1feb314d955ed06f25df5067c39d7f779
wchargin
added a commit
that referenced
this pull request
Mar 15, 2021
Summary: As the comment says, the histograms plugin used to manually downsample because the multiplexer data provider didn’t. As of #3272, it does, so this code is vestigial. Test Plan: Note that changing the default histogram sampling constants (in the same file) to very small numbers causes the frontend to show less data. Unit tests for the histograms plugin also cover this: removing downsampling from the multiplexer data provider causes the histogram tests to fail. wchargin-branch: histogram-rm-downsample
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary:
The
MultiplexerDataProvidernow respects itsdownsampleparameter,even though the backing
PluginEventMultiplexeralready performs itsown sampling. This serves two purposes:
downsampleargument, which is required.
that they will behave correctly with other data providers.
Test Plan:
Unit tests included. Note that changing the
_DEFAULT_DOWNSAMPLINGconstant in (e.g.) the scalars plugin to a small number (like
5) nowactually causes charts in the frontend to be downsampled.
wchargin-branch: data-mux-downsample