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

Python 3.11 #1955

Merged
merged 14 commits into from
Nov 2, 2022
Merged

Python 3.11 #1955

merged 14 commits into from
Nov 2, 2022

Conversation

filmor
Copy link
Member

@filmor filmor commented Sep 29, 2022

  • Add Python 3.11 type offsets
  • Add Python 3.11 to metadata and workflows
  • Improve geninterop script to handle new case in 3.11
  • Fix offsets for 3.11

What does this implement/fix? Explain your changes.

...

Does this close any currently open issues?

...

Any other comments?

...

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Ensure you have signed the .NET Foundation CLA
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

Comment on lines +479 to +482
var slots = new[] {
new TypeSpec.Slot(TypeSlotID.tp_traverse, subtype_traverse),
new TypeSpec.Slot(TypeSlotID.tp_clear, subtype_clear)
};
Copy link
Member

@lostmsu lostmsu Oct 18, 2022

Choose a reason for hiding this comment

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

This creates multiple sources of truth. Does editing post creation not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't. Python 3.11 rejects creating a type that has HAVE_GC set but not these slots. Rightly so :)

Copy link
Member

Choose a reason for hiding this comment

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

Can we have this array shared with the other place that used to use this function?

@filmor

This comment was marked as resolved.

@filmor

This comment was marked as resolved.

@lostmsu
Copy link
Member

lostmsu commented Oct 28, 2022

I can take a look once 3.11 hits Anaconda.

@lostmsu
Copy link
Member

lostmsu commented Oct 30, 2022

Re only clear on tp_dictoffset > 0. What is the scenario when it does not hold? I suspect the assert might be there to ensure something did not blow up earlier.

@filmor
Copy link
Member Author

filmor commented Oct 30, 2022

Re only clear on tp_dictoffset > 0. What is the scenario when it does not hold? I suspect the assert might be there to ensure something did not blow up earlier.

tp_dictoffset is only > 0 if there is actually a tp_dict, which is apparently not the case anymore for a lot of classes in Python 3.11. Fixing this fixed almost all segfaults (which were probably just unhandled .NET exceptions in the end). The only one left is the derived class in RecursiveInheritance requiring a similar setup for the slots (i.e. up-front instead of via modification) as I did in the other case. Here, it complains about tp_traverse.

@lostmsu
Copy link
Member

lostmsu commented Oct 30, 2022

OK, just wanted to ensure you investigated. Any specific type combinations that turn out to be an issue to make a test?

@filmor
Copy link
Member Author

filmor commented Oct 30, 2022

The only issue left is fixing (and reenabling tests) for Py_SetPythonHome (which is actually deprecated since 3.11). I'm not quite sure what's going on here, but something seems to fail in the encoding as it assumes in the test that the path is \0xfffd, so Unicode Replacement Character.

@filmor
Copy link
Member Author

filmor commented Nov 1, 2022

The remaining issue seems to be precisely calling Py_SetPythonHome with an empty string, which seems to corrupt the underlying storage. I'll investigate a bit more, but in the worst case, we should probably just forbid it.

@lostmsu
Copy link
Member

lostmsu commented Nov 1, 2022

LGTM, but I'll wait for the CI passing.

@filmor
Copy link
Member Author

filmor commented Nov 1, 2022

Wow, from my PoV this is actually a bug in Python itself:

https://github.com/python/cpython/blob/c0859743d9ad3bbd4c021200f4162cfeadc0c17a/Python/pathconfig.c#L255-L273

If Py_SetPythonHome was called with a non-empty string and is subsequently called with an empty one, it will always run PyMem_RawFree but only actually reset the pointer in .home if has_value is set, so if home && home[0] (i.e. non-empty string).

I'll try to create a proper bug-report for this, but for now I will just forbid setting PythonHome to an empty string. All of the other property setters have the same issue, AFAICT.

The issue occurs in our test-suite, because we usually start up with an empty PYTHONHOME, and then we try to update and later reset it. I don't know why this only occurs in Python 3.11, but the bug as such exists before that as well.

@filmor filmor force-pushed the python-3.11 branch 2 times, most recently from a4ac4f2 to f1be5b6 Compare November 2, 2022 07:30
@filmor filmor marked this pull request as ready for review November 2, 2022 08:05
@filmor filmor merged commit 6968d01 into pythonnet:master Nov 2, 2022
@filmor filmor deleted the python-3.11 branch November 2, 2022 08:06
@filmor
Copy link
Member Author

filmor commented Nov 2, 2022

I have investigated the issue further: It's indeed a regression in Python 3.11, as soon as it's fixed up-stream, I'll adjust our tests to always run again.

Martin-Molinero pushed a commit to Martin-Molinero/pythonnet that referenced this pull request Mar 15, 2024
Martin-Molinero added a commit to QuantConnect/pythonnet that referenced this pull request Mar 18, 2024
* Merge pull request pythonnet#1955 from filmor/python-3.11

Python 3.11

* Minor fix for datetime tz conversion

* Version bump to 2.0.29

---------

Co-authored-by: Benedikt Reinartz <[email protected]>
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.

4 participants