-
Couldn't load subscription status.
- Fork 48
tests: mark test that depends on multiprocess as Linux-only #838
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
Conversation
7f4d1a7 to
6a308f9
Compare
fb8d0b8 to
9391796
Compare
fork context is availablefork context are both available
src/taskgraph/generator.py
Outdated
| # processing. | ||
| if ( | ||
| platform.system() != "Linux" | ||
| or "fork" not in multiprocessing.get_all_start_methods() |
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.
I'm still a bit confused. Is fork ever not available on linux?
The commit message / PR title say "fix" but it's not clear what this fixes.
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.
I'm still a bit confused. Is fork ever not available on linux?
It certainly sounds like it should be; after writing the test I got to thinking that it's probably better to be cautious though? It probably doesn't matter in the real world, at least at this time. (I could be convinced to leave this part alone.)
The commit message / PR title say "fix" but it's not clear what this fixes.
I'll update this.
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.
Is fork ever not available on linux?
Technically you could make it non available but then the condition wouldn't catch it because python doesn't check if it can fork or not (and honestly, it's probable that python wouldn't even start without fork available on linux).
I got to thinking that it's probably better to be cautious though
I don't think it's worth making the condition more confusing than it need to be for a reader. If I see this without context it's going to make me think that somehow fork could be unavailable and probably overthink it...
FTR python does something like this to populate the start methods (I left the comment because it's interesting for the macos case where you were seeing crashes):
if sys.platform != 'win32':
_concrete_contexts = {
'fork': ForkContext(),
'spawn': SpawnContext(),
'forkserver': ForkServerContext(),
}
# bpo-33725: running arbitrary code after fork() is no longer reliable
# on macOS since macOS 10.14 (Mojave). Use spawn by default instead.
# gh-84559: We changed everyones default to a thread safeish one in 3.14.
if reduction.HAVE_SEND_HANDLE and sys.platform != 'darwin':
_default_context = DefaultContext(_concrete_contexts['forkserver'])
else:
_default_context = DefaultContext(_concrete_contexts['spawn'])
else: # Windows
_concrete_contexts = {
'spawn': SpawnContext(),
}
_default_context = DefaultContext(_concrete_contexts['spawn'])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.
Alright, I'm convinced!
fork context are both availablefork context are both available
9391796 to
24b2719
Compare
fork context are both availableAlso update the comment around this with more details, so I don't get confused next time...
24b2719 to
cc6a04c
Compare
Also update the comment around this with more details, so I don't get confused next time...