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

Always use the spawn start method for multiprocessing for wpt commands #29051

Merged
merged 1 commit into from May 20, 2021

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented May 19, 2021

No description provided.

@foolip
Copy link
Member

foolip commented May 20, 2021

@jgraham I've tried this locally and it doesn't fix #29024 if that was the hope?

@jgraham
Copy link
Contributor Author

jgraham commented May 20, 2021

I mean the hope was that we end up with consistent usage of multiprocesssing and in particular always stick to the safest context type. I thought it might affect #29024 but I"m not really surprised it doesn't given that this should be the default on macOS anyway.

@foolip
Copy link
Member

foolip commented May 20, 2021

https://docs.python.org/3/library/multiprocessing.html#multiprocessing.set_start_method says "it should be protected inside the if __name__ == '__main__' clause of the main module" which would require putting it in the root wpt script. I don't know why the documentation says that, and guess that calling it from a method called from that clause is fine, but 🤷

try:
multiprocessing.set_start_method('spawn')
except RuntimeError as e:
# This can happen if we call back into wpt having already set the context
Copy link
Member

Choose a reason for hiding this comment

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

This should be possible to avoid if we put it in the root wpt right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. But the reason I noticed this is that we have unit tests that call wpt.main instead of going via the command line. So I think we'd run the risk of those tests running under an unsupported configuration if we so this in a pre-main location.

I tried a test and I can't see that calling this in main vs if __name__ == "__main__" makes any difference to the behaviour; in each case the subprocess gets the same default spawn method. It seems like the important thing is really that it's called at the main entrypoint to the code; as long as we're confident everything goes through main it's all fine.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably the Python documentation says that as a slightly inaccurate way of saying "please make sure to call it always and early".

@foolip foolip merged commit 716c367 into master May 20, 2021
@foolip foolip deleted the wpt_spawn branch May 20, 2021 14:55
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.

None yet

3 participants