Make Everstore match existing dataset API #16
Closed
Conversation
This file contains 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
facebook-github-bot
added
the
CLA Signed
Summary: Pull Request resolved: facebookresearch/ClassyVision#16 Pull Request resolved: fairinternal/ClassyVision#2 As I tried to work on the downstream diffs some of the internal datasets which have sample fetch failures had a different API and this was causing me headaches. In particular, most of the dataset API is reimplemented for the everstore dataset (particularly the batching and transformation logic). This made the downstream work to refactor and ultimately delete this logic twice as complicated since I effectively had to do it twice (one for canonical datasets and once for the everstore datasets). This diff migrates the API for those datasets to fetch single samples and use the dataset wrappers like all of the other Classy datasets (so future migrations happen only once). This has performance implications since the original goal of separate batching logic was to issue multiple everstore requests simultaneously, so I also had to make a change to prefetch the upcoming samples to make sure performance was still good. This involved: 1. Everstore batching / transformation logic dealt with missing samples (None's), so I had to move some of that logic to the "is_not_none" filter function and add a ignore none's option to the transforms. 2. Modifying all downstream datasets (e.g. the visual relevance team's datasets) 3. Adding prefetching to the Everstore dataset for performance. 4. Found some small CLI changes to the internal benchmark to make it easier to use by displaying samples when needed. I then verified that performance was fine. The performance turned out to be faster than I measured on the previous benchmark, I'm still not sure why this is, but I added functionality to the benchmark to print out a tensor for visual inspection and the data appears to be fine. As a note, I was trying to figure out if we can switch to spawn here and ran into a host of problems which was what was blocking this work, so I will save that for a later diff. Differential Revision: D17266435 fbshipit-source-id: 948eb535f8df7379dadff7f19f27ce86e36bee19
aadcock
force-pushed
the
export-D17266435
branch
from
1707585
to
4560fee
Compare
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 28, 2019
Summary: Pull Request resolved: #23 Pull Request resolved: #16 Pull Request resolved: fairinternal/ClassyVision#2 As I tried to work on the downstream diffs some of the internal datasets which have sample fetch failures had a different API and this was causing me headaches. In particular, most of the dataset API is reimplemented for the everstore dataset (particularly the batching and transformation logic). This made the downstream work to refactor and ultimately delete this logic twice as complicated since I effectively had to do it twice (one for canonical datasets and once for the everstore datasets). This diff migrates the API for those datasets to fetch single samples and use the dataset wrappers like all of the other Classy datasets (so future migrations happen only once). This has performance implications since the original goal of separate batching logic was to issue multiple everstore requests simultaneously, so I also had to make a change to prefetch the upcoming samples to make sure performance was still good. This involved: 1. Everstore batching / transformation logic dealt with missing samples (None's), so I had to move some of that logic to the "is_not_none" filter function and add a ignore none's option to the transforms. 2. Modifying all downstream datasets (e.g. the visual relevance team's datasets, also made max_samples -> num_samples to match all other datasets) 3. Adding prefetching to the Everstore dataset for performance. 4. Found some small CLI changes to the internal benchmark to make it easier to use by displaying samples when needed. I then verified that performance was fine. The performance turned out to be faster than I measured on the previous benchmark, I'm still not sure why this is, but I added functionality to the benchmark to print out a tensor for visual inspection and the data appears to be fine. As a note, I was trying to figure out if we can switch to spawn here and ran into a host of problems which was what was blocking this work, so I will save that for a later diff. Reviewed By: vreis Differential Revision: D17266435 fbshipit-source-id: 337e8dcd4558d58fc28c5c47f99ff681addccc8b
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 26, 2019
Summary: Pull Request resolved: fairinternal/ClassyVision#16 Pull Request resolved: #120 This is a simple diff, I think that the constructor should own verifying the inputs rather than the parse config function in case someone uses a custom parsing. Reviewed By: mannatsingh Differential Revision: D18138588 fbshipit-source-id: bbf3ca780f96a5d3c47ab526552b77e9e1157910
vaibhava0
pushed a commit
to vaibhava0/ClassyVision
that referenced
this pull request
Jan 19, 2021
Summary: Pull Request resolved: fairinternal/ClassyVision#16 Pull Request resolved: facebookresearch#120 This is a simple diff, I think that the constructor should own verifying the inputs rather than the parse config function in case someone uses a custom parsing. Reviewed By: mannatsingh Differential Revision: D18138588 fbshipit-source-id: bbf3ca780f96a5d3c47ab526552b77e9e1157910
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
As I tried to work on the downstream diffs some of the internal datasets which have sample fetch failures had a different API and this was causing me headaches. This migrates the API for those datasets to fetch single samples and instead prefetch the data to avoid complications around batching, etc.
This involved:
I then verified that performance was fine. The performance turned out to be faster than I measured on the previous benchmark, I'm still not sure why this is, but I added functionality to the benchmark to print out a tensor for visual inspection and the data appears to be fine.
As a note, I was trying to figure out if we can switch to spawn here and ran into a host of problems which was what was blocking this work, so I will save that for a later diff.
Differential Revision: D17266435