facebook / buck

A fast build system that encourages the creation of small, reusable modules over a variety of platforms and languages.
https://buck.build
Apache License 2.0
8.56k stars 1.16k forks source link

make allocators and sanitizers work for processes created with multiprocessing's spawn method in dev mode #2660

Open yifuwang opened 2 years ago

yifuwang commented 2 years ago

Summary: The first attempt (D30802446) overlooked that fact that the interpreter wrapper is not an executable (for execv) on Mac, and introduced some bugs due to the refactoring. The attempt 2 addressed the issues, and isolated the effect of the change to only processes created by multiprocess's spawn method on Linux.

Problem

Currently, the entrypoint for in-place Python binaries (i.e. built with dev mode) executes the following steps to load system native dependencies (e.g. sanitizers and allocators):

The steps work as intended for single process Python programs. However, when a Python program spawns child processes, the child processes will not load native dependencies, since they simply execv's the vanilla Python interpreter. A few examples why this is problematic:

Many if not most ML use cases "bans" dev mode because of these problems. It is very unfortunate considering the developer efficiency dev mode provides. In addition, a huge amount of unit tests have to run in a more expensive build mode because of these problems.

For an earlier discussion, see this post.

Solution

Move the system native dependencies loading logic out of the Python binary entrypoint into an interpreter wrapper, and set the interpreter as sys.executable in the injected prologue:

Alternative Considered

One alternative considered is to simply not removing system native dependencies from LD_PRELOAD, so they are present in the spawned processes. However, this can cause some linking issues, which were perhaps the reason LD_PRELOAD was restored in the first place.

References

An old RFC for this change: D16210828 The counterpart for opt mode: D16350169

Reviewed By: fried

facebook-github-bot commented 2 years ago

This pull request was exported from Phabricator. Differential Revision: D31106794

facebook-github-bot commented 2 years ago

This pull request was exported from Phabricator. Differential Revision: D31106794

facebook-github-bot commented 2 years ago

This pull request was exported from Phabricator. Differential Revision: D31106794

facebook-github-bot commented 2 years ago

This pull request was exported from Phabricator. Differential Revision: D31106794

facebook-github-bot commented 2 years ago

This pull request was exported from Phabricator. Differential Revision: D31106794

facebook-github-bot commented 2 years ago

This pull request was exported from Phabricator. Differential Revision: D31106794