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

Windows numpy hang #2

Merged
merged 3 commits into from
Apr 25, 2024
Merged

Windows numpy hang #2

merged 3 commits into from
Apr 25, 2024

Conversation

ctrueden
Copy link
Contributor

This fixes a hang on Windows where import numpy run too late, after Appose already starts reading from stdin, never returns. The relevant bug is numpy/numpy#24290. To work around this, we can simply do a try/except on import numpy at the very beginning of the python_worker.py, in the bundled appose-python. The proper fix later will be for Appose to support passing an init script so that users of the library can simply pass a script with only import numpy at environment construction time. But for now, this hack works to make SAMJ work on Windows.

The numpy library has a bug when importing on Windows with stdin already
open, which causes numpy to hang, because it also tries to read from
stdin internally. See: numpy/numpy#24290

To avoid this, a proper fix will be to add a feature to Appose to pass
some kind of init script that is run early, before the Appose pipes get
established. Then it would be as simple as passing a single line
`import numpy` as the init script in code that needs to avoid this bug.

But because SAMJ currently ships its own zipped copy of appose-python,
we add the following block of code to appose-python's python_worker.py:

    # HACK: Avoid numpy hang on Windows systems when stdin is open.
    # numpy/numpy#24290
    if sys.platform.startswith("win"):
        try:
            import numpy
        except Exception as e:
            print(f"[WARNING] Failed to import numpy eagerly: {e}", file=sys.stderr)

(You can't see the patch directly here since it's inside the zip file.)
It is already false by default in the interface.
@ctrueden
Copy link
Contributor Author

@carlosuc3m The CI check fails because the main branch is currently broken.

I already uploaded this fix (which branches from the previously uploaded build, not the current tip of main) to the SAMJ update site.

@carlosuc3m carlosuc3m merged commit 31a3697 into main Apr 25, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants