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

[WIP] Only run writer close test once #260

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

orionr
Copy link
Contributor

@orionr orionr commented Oct 27, 2018

This had some issues in our internal PyTorch testing infrastructure.

@lanpa
Copy link
Owner

lanpa commented Oct 28, 2018

The loop was added intentionally. What is the issue?

@orionr
Copy link
Contributor Author

orionr commented Oct 29, 2018

I'm not 100% sure this solves the issue (still testing internally) but we have some internal stress tests that fail on this test with the loop. I still need to verify this fixes the issue, but can you explain the reason for having the loop? Was this essentially a stress test?

@orionr orionr changed the title Only run writer close test once [WIP] Only run writer close test once Oct 29, 2018
@lanpa
Copy link
Owner

lanpa commented Oct 29, 2018

#108

I remember that I tried to fix this long time ago and suspected that it might be related to python's GIL.
The maximum open/close allowed are different on different OS.

@orionr
Copy link
Contributor Author

orionr commented Oct 29, 2018

That's possible. Just landed internal changes to see if this fixes the issue we were seeing or not. I'll report back.

@orionr
Copy link
Contributor Author

orionr commented Oct 30, 2018

Looks like this did fix the internal case. It's possible that we could have a few loops, but the large number was definitely an issue.

@lanpa lanpa merged commit 732e7d1 into lanpa:master Oct 30, 2018
@lanpa
Copy link
Owner

lanpa commented Oct 30, 2018

Yeah, I believe it needs a major rewrite with the multiprocessing module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants