Run reports in a separate Python process. #41
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
plietar
force-pushed
the
mrc-5228
branch
2 times, most recently
from
0053fd1
to
cedb387
Compare
This comment was marked as outdated.
This comment was marked as outdated.
plietar
force-pushed
the
mrc-5227
branch
3 times, most recently
from
92c0ecf
to
7f85351
Compare
richfitz
approved these changes
May 3, 2024
| from outpack.util import openable_temporary_file | ||
|
|
||
|
|
||
| def run_in_sandbox(target, args=(), cwd=None, syspath=None): |
There was a problem hiding this comment.
We use callr for this sort of manoeuvre in R - does python not have something we can pull in to do this for us? This way is fine though!
There was a problem hiding this comment.
All I could find is the multiprocessing module, but it's a bit inconvenient to use and puts a burden on the caller to setup their script in the right way.
| except BaseException as e: | ||
| # This allows the traceback to be pickled and communicated out of the | ||
| # the sandbox. | ||
| pickling_support.install(e) |
The runpy module we've been using to run reports provides very little isolation. In particular, "any side effects (such as cached imports of other modules) will remain in place after the functions have returned". This means that if a user splits their report into multiple files, with one file importing another, the latter will be cached and never reloaded by Python, even if it is modified. Similarly, if the module was already loaded before, the report runs against old code that does not match the files included in the packet. Using a separate subprocess gives us a much stronger isolation, including all global variables and not sharing any of previously loaded modules. It means a user can run a report, modify some of the imported code, run the report again and have this behave as expected. The sandbox is implemented by pickling the function and its argument into a file, starting a new Python process which unpickles the file and calls the target. Similarly, the function's return value, or any exception it may throw, it pickled to an output file, which is read by the parent. I tried to use the multiprocessing module to implement this, but it puts inconvenient requirements on the way the top-level source file is written. The module re-evaluates the file in the subprocess, meaning its contents must check for `__name__ == "__main__"` to avoid doing the work again. It also doesn't work too well with the REPL. Using our own implementation fixes these issues. The Packet class is refactored a little and its scope is reduced to just creating the packet and its metadata, without inserting it into the repository. The insertion is now handled by a separate function. This is done as a way of distinguishing what happens in the child process and what happens in the parent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The runpy module we've been using to run reports provides very little isolation. In particular, "any side effects (such as cached imports of other modules) will remain in place after the functions have returned". This means that if a user splits their report into multiple files, with one file importing another, the latter will be cached and never reloaded by Python, even if it is modified. Similarly, if the module was already loaded before, the report runs against old code that does not match the files included in the packet.
Using a separate subprocess gives us a much stronger isolation, including all global variables and not sharing any of previously loaded modules. It means a user can run a report, modify some of the imported code, run the report again and have this behave as expected.
The Packet class is refactored a little and its scope is reduced to just creating the packet and its metadata, without inserting it into the repository. The insertion is now handled by a separate function. This is done as a way of distinguishing what happens in the child process and what happens in the parent.