Skip to content

Conversation

cjdoris
Copy link
Collaborator

@cjdoris cjdoris commented Jul 15, 2025

Hopefully fixes this issue: #641 (comment)

@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 15, 2025

If this works, I don't fully understand why, because we never seem to actually ever dereference a whole PyTypeObject, we use UnsafePointers to access individual fields, which should be fine, since these fields are available in all supported versions of Python.

Perhaps it's a bug in UnsafePointers.jl.

@cjdoris cjdoris force-pushed the hotfix-pytypeobject-fields branch from 0f736a6 to d3384ff Compare July 15, 2025 19:39
@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 15, 2025

OK good - what I did didn't actually fix the problem, the bus error persists, so I have reverted the commit. Testing all minor versions of Python to see how bad this is.

@MilesCranmer
Copy link
Contributor

MilesCranmer commented Jul 15, 2025

Python 3.8 is EOL, likely good to remove anyways? pip will resolve to an earlier version of juliacall anyways, should someone want it. And that version would still be compatible with the old python type.

i.e., https://endoflife.date/python

@cjdoris cjdoris force-pushed the hotfix-pytypeobject-fields branch from 4f3f872 to 052df20 Compare July 15, 2025 19:59
@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 15, 2025

Some trivial changes resulted in segfaults on linux on Python 3.9 and 3.11. Reverting those to see what happens.

@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 15, 2025

Python 3.8 is EOL, likely good to remove anyways?

Yep agreed - I do aim to only support supported Python versions, just hadn't spotted that 3.8 was EOL now.

@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 15, 2025

After reverting, everything passed. Undoing the revert again...


# Python 3.13+ fields
tp_versions_used::Cushort = 0
versions_used::UInt16 = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

julia> Cushort === UInt16
true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not necessarily on all systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, which systems would this actually not be true for?

Choose a reason for hiding this comment

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

In Julia the "C*" aliases are best guesses based on observed experience of common compilers (mainly gcc and clang), and in this specific case Cushort is indeed always UInt16: https://github.com/JuliaLang/julia/blob/11eeed32e8fe3cd24c8439fbe2220d3f4183f306/base/ctypes.jl#L27. But the reality is that the C standard doesn't specify fixed size for types, and leaves the implementation up to compilers which can do what the heck they want. "short" is only expected to be at least 16 bits, but a compiler making "short" 17 bits could be fully standard-compliant.

@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 15, 2025

Now just Ubuntu Python 3.9 failing (segfault). Rerunning...

@cjdoris
Copy link
Collaborator Author

cjdoris commented Jul 15, 2025

All passed after rerunning. I think the real issue is just that we didn't add PYTHON_JULIACALL_HANDLE_SIGNALS=yes when we made the tests multithreaded - and this is nothing to do with the recent issues with PyTypeObject.

@MilesCranmer
Copy link
Contributor

Cool; makes sense

@cjdoris cjdoris changed the title Hotfix pytypeobject fields Test with PYTHON_JULIACALL_HANDLE_SIGNALS=yes Jul 15, 2025
@cjdoris cjdoris merged commit a0d4ee8 into main Jul 15, 2025
14 checks passed
@cjdoris cjdoris deleted the hotfix-pytypeobject-fields branch July 15, 2025 21:29
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.

3 participants