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

make sure only the thread that holds the GIL does Python stuff #107

Merged
merged 3 commits into from
Jul 13, 2024

Conversation

chillenb
Copy link
Contributor

@chillenb chillenb commented Jul 12, 2024

This is one way to resolve #106.

You could also redefine

    check_signal_() = []() {
        PyGILState_STATE gstate;
        gstate = PyGILState_Ensure();
        if (PyErr_CheckSignals() != 0)
            throw py::error_already_set();
        PyGILState_Release(gstate);
    };

@hczhai
Copy link
Contributor

hczhai commented Jul 12, 2024

Thanks for identifying and solving this problem. I have the following suggestions.

  1. the unnecessary {, }, and the comment line should be removed, so only 4 lines should be necessary.
  2. In 0708415 I added the driver.get_csf_coefficients into the Python unit test. On GitHub actions, these tests run on Python 3.12, and you will see the segment fault in the log. You may merge that commit into this PR and this PR is supposed to fix the unit tests.
  3. It seems that the problem was only reproducible in Python 3.12 (so it seems that they are making GIL more awkward to use in 3.12 so that they would have more reason to remove it in 3.13).

@chillenb
Copy link
Contributor Author

OK, I rebased off of 0708415 and removed the braces/comments. Let's hope the tests pass!!

It seems that the problem was only reproducible in Python 3.12 (so it seems that they are making GIL more awkward to use in 3.12 so that they would have more reason to remove it in 3.13).

This makes sense 😆 It would be nice if they had gotten rid of the GIL going from Python 2 to 3--that was quite painful and GIL problems wouldn't have made it much worse.

@hczhai hczhai merged commit c33cf18 into block-hczhai:master Jul 13, 2024
31 checks passed
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.

segfault in driver.get_csf_coefficients
2 participants