This repository was archived by the owner on Oct 31, 2023. It is now read-only.
fix silly numpy import/fork bug#24
Merged
Merged
Conversation
facebook-github-bot
added
the
CLA Signed
heiner
approved these changes
Feb 5, 2022
| // numpy forks during import, and pybind can trigger a numpy import | ||
| // during serialization (just for checking if the input is a numpy array). | ||
| // forks generally break moolib, so do it here where it's safe | ||
| py::detail::npy_api::get(); |
Contributor
There was a problem hiding this comment.
One alternative would be to do that in Python, namely in __init__.py
Contributor
Author
There was a problem hiding this comment.
I feel like this is neater, as it runs the code that actually triggers the issue during serialization. If we imported in init.py, we'd just be hoping that it imports the same numpy and resolves the issue (which it probably would), but this is safer.
Contributor
|
If I understand this correctly, the issue is likely in the linear algebra library numpy uses, specifically in MKL? So not strictly speaking a numpy issue even. |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
This is pretty silly, but during serialization we check if the input is a numpy array (despite #17). For this check, pybind needs to import numpy. During import, my local version of numpy calls fork. From googling, this is seen as a no-no, but it does it regardless. I've not investigated why it does the fork.
Forking during serialization breaks moolib with the usual dont-fork fatal error message (and setting the MOOLIB_ALLOW_FORK env var results in a deadlock).
There is no issue if the user already imported numpy (or pytorch, as that also pulls in numpy). However, if they didn't, even the most basic usage of moolib results in a fatal error.
This fix calls the pybind code that in turn imports numpy during Moolib module initialization, thus numpy is already imported during serialization.