indygreg / PyOxidizer

A modern Python application packaging and distribution tool
Mozilla Public License 2.0
5.47k stars 239 forks source link

Incorrect python call in MainPythonInterpreter::run_multiprocessing #603

Open vxgmichel opened 2 years ago

vxgmichel commented 2 years ago

While messing with the multiprocessing spawn start method on linux, I got this error:

  File "multiprocessing.spawn", line 115, in spawn_main
TypeError: 'dict' object cannot be interpreted as an int

Even though I read in the docs that:

the spawn start method is known to be buggy with PyOxidizer except on Windows

.., I noticed what seems to be a bug in MainPythonInterpreter::run_multiprocessing:

https://github.com/indygreg/PyOxidizer/blob/e063d3d6a9519e1b10e35b5117392cd9980de0fe/pyembed/src/interpreter.rs#L563

This effectively calls spawn_main(kwargs). Shouldn't this be spawn_main(**kwargs) instead?

I built the project with this diff and it seems to fix my issue:

diff --git a/pyembed/src/interpreter.rs b/pyembed/src/interpreter.rs
index 8f1deb3f..7fc9954a 100644
--- a/pyembed/src/interpreter.rs
+++ b/pyembed/src/interpreter.rs
@@ -560,7 +560,7 @@ impl<'interpreter, 'resources> MainPythonInterpreter<'interpreter, 'resources> {
             }

             let spawn_module = py.import("multiprocessing.spawn")?;
-            spawn_module.getattr("spawn_main")?.call1((kwargs,))?;
+            spawn_module.getattr("spawn_main")?.call((), Some(kwargs))?;

             Ok(0)
         })
indygreg commented 2 years ago

Yikes! This is buggy argument passing to a Python function and your patch looks like the right fix. I'll commit a fix next time I sit down to hack on PyOxidizer.

indygreg commented 2 years ago

Thanks for the bug report and the investigation! (I wish we had better test coverage of multiprocessing functionality: there have been few bugs in it!)

tsibley commented 1 year ago

Ran into this myself recently.

fooblahblah commented 10 months ago
spawn_module.getattr("spawn_main")?.call((), Some(kwargs))?;

This is awesome. I forked and gave this a try and am able to do what I wanted to do with multiprocessing spawn Thanks!