-
Notifications
You must be signed in to change notification settings - Fork 121
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
update: adding a test to check for circular imports #812
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #812 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 129 129
Lines 8388 8388
Branches 1870 1870
=========================================
Hits 8388 8388 ☔ View full report in Codecov by Sentry. |
Should consistent start methods across OS's be confirmed with https://docs.python.org/3/library/multiprocessing.html#multiprocessing.set_start_method |
Do you mean that we should be using spawn on linux? Why? It doesn't seem it would make a difference here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change!
While talking offline, Abe brought up the point that the parent process could have imports that could carry over to the children with 'fork'. I'm not sure about that, but it seems fairly safe to set them all to 'spawn'. |
Note: use the prefix |
Sorry, I didn't see this in time. |
Issue #, if available:
Description of changes:
Testing done:
I reset back to where we had the issue with eed59ff change, and then verified that I could detect the issue regardless of the order that the modules were tested in.
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.