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

MAINT: Help boost::python libraries at least not crash #20616

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

seberg
Copy link
Member

@seberg seberg commented Dec 17, 2021

This adds an almost random "sanity check" to PyArray_EquivTypes
for the sole purpose of allowing boost::python compiled libs to
not crash.
boost::python is buggy, it needs to be fixed. This may break them
(I am unsure), because some conversions which used to work may fail
here (if they worked, they only worked because random type data may
have matched up correctly for our scalar types).

We could error, or warn or... but I hope boost::python will just fix
this soon enough and future us can just delete the whole branch.

Replaces gh-20507

This adds an almost random "sanity check" to `PyArray_EquivTypes`
for the sole purpose of allowing boost::python compiled libs to
_not crash_.
boost::python is buggy, it needs to be fixed.  This may break them
(I am unsure), because some conversions which used to work may fail
here (if they worked, they only worked because random type data may
have matched up correctly for our scalar types).

We could error, or warn or... but I hope boost::python will just fix
this soon enough and future us can just delete the whole branch.

Replaces numpygh-20507
@seberg seberg added the 09 - Backport-Candidate PRs tagged should be backported label Dec 17, 2021
@seberg
Copy link
Member Author

seberg commented Dec 17, 2021

If we do this and if there will be another 1.21.x, we should also backport it to 1.21.x.

*
* boost::python has/had a bug effectively using EquivTypes with
* `type(arbitrary_obj)`. That is clearly wrong as that cannot be a
* `PyArray_Descr *`. We assume that `type(type(type(arbitrary_obj))`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, type(type(type(obj))) should be right for the sanity check :).

@charris
Copy link
Member

charris commented Dec 17, 2021

Thanks Sebastian, seems harmless enough, but I'm not particularly happy about working around what looks like a boost bug.

Let's not backport anymore stuff if we can help it, it's starting to look like 1.22.1 and we haven't even made it to rc3 yet :)

@charris charris added component: numpy._core and removed 09 - Backport-Candidate PRs tagged should be backported labels Dec 17, 2021
@charris charris added this to the 1.21.5 release milestone Dec 17, 2021
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Dec 17, 2021
@seberg
Copy link
Member Author

seberg commented Dec 17, 2021

Sounds fair, I am in no rush to backport (the boost users should also validate it quickly to be frank) and it would be nicer if boost::python just fixes things and we revert...

@seberg seberg deleted the hack-for-boost-python branch December 17, 2021 21:29
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Dec 17, 2021
@charris charris removed this from the 1.21.5 release milestone Dec 17, 2021
@dwpaley
Copy link

dwpaley commented Dec 20, 2021

@seberg Thanks for doing this. I can confirm this commit resolves the segfaults in boostorg/python#376 and cctbx/cctbx_project#627.

dwpaley added a commit to cctbx/cctbx_project that referenced this pull request Jan 4, 2022
This reflects the resolution of #627 as discussed in several other issues
and PRs:

- boostorg/python#376
- numpy/numpy#20507
- numpy/numpy#20616

Leaving this "bibliography" here because the fix in numpy PR 20616 is
considered temporary; thus someday we may have to revisit this to fix the
underlying bug in boost::python.

Co-authored-by: Billy Poon <bkpoon@lbl.gov>
dwpaley added a commit to cctbx/cctbx_project that referenced this pull request Jan 5, 2022
This reflects the resolution of #627 as discussed in several other issues
and PRs:

- boostorg/python#376
- numpy/numpy#20507
- numpy/numpy#20616

Leaving this "bibliography" here because the fix in numpy PR 20616 is
considered temporary; thus someday we may have to revisit this to fix the
underlying bug in boost::python.

Co-authored-by: Billy Poon <bkpoon@lbl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants