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.

Sign up for GitHub

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

Jump to bottom

Fix dataloader hang with large sampler #48669

Closed
wants to merge 1 commit into from
Closed

Fix dataloader hang with large sampler #48669

wants to merge 1 commit into from

Conversation

ssnl
Copy link
Collaborator

@ssnl ssnl commented Dec 1, 2020

Fixes #48666

ssnl
ssnl commented Dec 1, 2020
View reviewed changes
torch/utils/data/dataloader.py
# index_queue.cancel_join_thread()
# Need to `cancel_join_thread` here!
# See sections (2) and (3b) above.
index_queue.cancel_join_thread()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the only relevant code change. the rest is just doc and test

@zhangguanheng66 zhangguanheng66 added module: dataloader Related to torch.utils.data.DataLoader and Sampler triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Dec 1, 2020
VitalyFedyunin
VitalyFedyunin approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@VitalyFedyunin VitalyFedyunin left a comment •
edited

Choose a reason for hiding this comment

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

The code looks good, but how slow is the test?
Nevermind, it is joined right away.

facebook-github-bot
facebook-github-bot reviewed Dec 1, 2020
View reviewed changes
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.

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ssnl
Copy link
Collaborator Author

ssnl commented Dec 1, 2020 •
edited

@VitalyFedyunin

It runs in 3s on my linux box

> python ../pytorch/test/run_test.py -i test_dataloader -v  -- TestDataLoader.test_large_sampler_indices
Fail to import hypothesis in common_utils, tests are not derandomized
Test executor: [XXXXXXXXXXXXXXXXXXXX]
Selected tests: test_dataloader
Running test_dataloader ... [2020-12-01 17:37:46.778711]
Fail to import hypothesis in common_utils, tests are not derandomized
test_large_sampler_indices (__main__.TestDataLoader) ... Fail to import hypothesis in common_utils, tests are not derandomized
Fail to import hypothesis in common_utils, tests are not derandomized
ok

----------------------------------------------------------------------
Ran 1 test in 3.528s

OK

@dr-ci
Copy link

dr-ci bot commented Dec 1, 2020 •
edited

💊 CI failures summary and remediations

As of commit 375cd60 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed 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 or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 1 time.

@facebook-github-bot
Copy link
Contributor

@VitalyFedyunin merged this pull request in 4abca90.

@facebook-github-bot
Copy link
Contributor

@VitalyFedyunin merged this pull request in 4abca90.

shaibagon pushed a commit to shaibagon/pytorch that referenced this pull request Dec 3, 2020
@ssnl @shaibagon
Fix dataloader hang with large sampler (pytorch#48669)
d5abf18 
Summary:
Fixes pytorch#48666

Pull Request resolved: pytorch#48669

Reviewed By: zhangguanheng66

Differential Revision: D25255763

Pulled By: VitalyFedyunin

fbshipit-source-id: d06421f52bb1d00cdf8025f1a2ba0d1f9284731a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@VitalyFedyunin VitalyFedyunin VitalyFedyunin approved these changes

Assignees
No one assigned
Labels
cla signed Merged module: dataloader Related to torch.utils.data.DataLoader and Sampler open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[dataloader] hang on python exit when has iter ref and sampler yields large indices
5 participants
@ssnl @facebook-github-bot @VitalyFedyunin @zhangguanheng66 @pytorchbot