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

Relax sampler check in BatchSampler #38403

Closed
wants to merge 3 commits into from
Closed

Conversation

SsnL
Copy link
Collaborator

@SsnL SsnL commented May 13, 2020

Since the check was added in #6249, one can not pass an iterable as a sampler to the data loader anymore, which was a very handy feature (e.g., #1337). I think the check should be removed for two-fold reasons:

  1. It is too strict. There is no reason that it should not be a general iterable.
  2. It is inconsistent. In DataLoader (the main place where people use samplers), you can pass a general iterable as batch_sampler but not sampler due to this check.

@SsnL SsnL requested a review from apaszke as a code owner May 13, 2020
@dr-ci
Copy link

@dr-ci dr-ci bot commented May 13, 2020

💊 CI failures summary and remediations

As of commit 4f5de83 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 5 times.

Copy link
Member

@soumith soumith left a comment

lgtm cc: @colesbury for any reservations

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@colesbury
Copy link
Member

@colesbury colesbury commented May 13, 2020

lgtm too

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 14, 2020

@soumith merged this pull request in b5868b2.

@SsnL SsnL deleted the dl_sampler_check branch May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants