-
Notifications
You must be signed in to change notification settings - Fork 46
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
FIX support for frozen executable on all platforms #375
base: master
Are you sure you want to change the base?
Conversation
This sounds awesome! Hoping that |
Are we still needed here? I can probably try to get it running, although I scrapped my original windows vm since then, but we now have github actions instead, so probably something to do there. Can't remember if it was also a problem on other platforms too, but that seems likely. |
I think we need an integration test that runs a minimal loky-based program through pyinstaller and check that it runs correctly. And we also need a docstring for this: https://github.com/joblib/loky/pull/375/files#r1061777509. |
I tried this PR with pyinstaller on the following test program named import loky
if __name__ == "__main__":
loky.freeze_support()
e = loky.get_reusable_executor()
print(sum(e.map(lambda x: x ** 2, range(1000))))
and it creates many subprocesses that continuously output the result on stdout even after the parent process has completed (I had to I tried both at 95ae81c and after the merge of the This is running:
on Linux. Note: I cannot get pyinstaller to work on my local macOS machine because of the problem with the |
Hum, I re-read the original description of the issue and maybe parsing the command line args to raise the |
I tried just that and could not get it to work either. Or maybe it's working as expected but the error messages on stderr are not expected. |
Thanks @tomMoral! I confirm that the latest commit make my minimal snippet from #375 (comment) work fine! |
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.
A few clarification comments
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.
linting....
Summary of the failures of my new test:
I will try to see if I can link some of those problems to known issues in the pyinstaller issue tracker. |
Some of the latest changes cause the resource tracker start-up to crash on all windows jobs: Traceback (most recent call last):
File "X:\loky\loky\backend\spawn.py", line 283, in main
exitcode = _main(fd, parent_sentinel=parent_sentinel)
File "X:\loky\loky\backend\spawn.py", line 301, in _main
prepare(preparation_data)
File "X:\loky\loky\backend\spawn.py", line 195, in prepare
_resource_tracker._fd = msvcrt.open_osfhandle(handle, 0)
OSError: [Errno 9] Bad file descriptor |
Yes this is expected. The strategy put in place by |
3fa306f
to
8e4a3ca
Compare
After (too much) investigation, I think this is actually related to Not sure how to fix this yet... |
It's weird, I tried to use |
For information, I tried to see if the refactoring in this PR would potentially fix the |
|
+1 for opening an issue upstream on the pyinstaller issue tracker and xfailing the windows test case in loky in the mean time. Please update the changelog accordingly to document that freeze support for windows is still a work in progress. |
I believe you meant to merge or rebase with current |
Actually no need, because the latest CI config of |
Unfortunately, this PR does not fix the |
There is another, maybe related failure in |
Actually, the ================================== FAILURES ===================================
________________________ test_kill_process_tree[True] _________________________
use_psutil = True
@pytest.mark.parametrize("use_psutil", [True, False])
def test_kill_process_tree(use_psutil):
psutil = pytest.importorskip("psutil")
event = ctx_loky.Event()
p = ctx_loky.Process(target=_run_nested_delayed, args=(4, 1000, event))
p.start()
# Wait for all the processes to be launched
if not event.wait(30):
kill_process_tree(p, use_psutil=use_psutil)
> raise RuntimeError(
"test_kill_process_tree was not able to launch "
"all nested processes."
)
E RuntimeError: test_kill_process_tree was not able to launch all nested processes.
event = <Event at 0x20e3e77e9d0 unset>
p = <LokyProcess name='LokyProcess-20' pid=1792 parent=7720 stopped exitcode=15>
psutil = <module 'psutil' from 'D:\\a\\1\\s\\.tox\\py311\\Lib\\site-packages\\psutil\\__init__.py'>
use_psutil = True
tests\test_loky_backend.py:721: RuntimeError
---------------------------- Captured stdout call -----------------------------
--------------------------------------------------------------------------------
None failed with traceback:
--------------------------------------------------------------------------------
Traceback (most recent call last):
File "D:\a\1\s\loky\backend\spawn.py", line 298, in main
exitcode = _main(fd, parent_sentinel=parent_sentinel)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\a\1\s\loky\backend\spawn.py", line 317, in _main
self = pickle.load(from_parent)
^^^^^^^^^^^^^^^^^^^^^^^^
EOFError: Ran out of input |
This is an attempt to fix
loky
for frozen support for all plateforms. (cc #236 , joblib/joblib#1002). (I got nerd snipped 😅 )This basically adds a
freeze_support
function that allows starting the worker with the right executable if it is called right afterif __name__ == "__main__":
.This PR also cleanups and refactor the command line generation, to avoid some duplicated code.
I used the following scripts to test and reproduce the error in pure
loky
:this gives me the output:
Note that the error is due to the fact that actually, the resource tracker from
cpython
is also broken. I am not totally sure why it is actually started but this is not a problem for running the code.If the code from this PR actually fixes the issue, it could be ported to python to make the frozen support working with all platforms/start methods and with the resource tracker.
As a disclaimer, I did not test this on windows and I would be very glad to have some feedbacks from the original reporters. Maybe from @samuelstjean or @gdoras if you are still around. Sorry for the slow response....
TODO before merge: