Skip to content

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.

@dr-ci
Copy link

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.

@ssnl ssnl force-pushed the dl_sampler_check branch from a06e5ca to 4f5de83 Compare May 13, 2020 17:06
Copy link
Contributor

@soumith soumith left a comment

Choose a reason for hiding this comment

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

lgtm cc: @colesbury for any reservations

Copy link
Contributor

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

Choose a reason for hiding this comment

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

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

@colesbury
Copy link
Member

lgtm too

@facebook-github-bot
Copy link
Contributor

@soumith merged this pull request in b5868b2.

@ssnl ssnl deleted the dl_sampler_check branch May 14, 2020 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants