Start to implement PyType_FromSpec type approach #1196

koubaa · 2020-08-01T17:18:42Z

What does this implement/fix? Explain your changes.

Begin to address #1033. Just beginning with this but wanted to submit a PR to solicit input about direction/approach.
...

Does this close any currently open issues?

#1033
...

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
Add yourself to AUTHORS
Updated the CHANGELOG

koubaa · 2020-08-01T17:20:04Z

@filmor FYI. Clearly I need to fix runtime issues, do some cleanup and move things around but this is my general approach.

filmor · 2020-08-01T23:01:36Z

Thanks for working on this! This could simplify compatibility with different Python versions a lot 👍

src/runtime/typemanager.cs

lostmsu · 2020-08-02T05:07:48Z

src/runtime/typemanager.cs

+        internal struct PY_TYPE_SLOT
+        {
+            public long slot; //slot id, from typeslots.h
+            public IntPtr func; //function pointer of the function implementing the slot
+        }
+
+        [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Ansi)]
+        internal class PyTypeSpecOffset


Ideally, these extra types should go into their own files.

@lostmsu I intend to do it after I get things working as part of the final cleanup

lostmsu · 2020-08-02T05:08:41Z

src/runtime/typemanager.cs

+
+            public static IntPtr AllocPyTypeSpec(string typename, int obSize, int obFlags, IntPtr slotsPtr)
+            {
+                byte[] ascii = System.Text.Encoding.ASCII.GetBytes(typename);


.NET supports Unicode type names. Not sure about Python here.

@lostmsu I think its possible. Maybe we should use UTF8 here - I should add a test to see for sure

src/runtime/typemanager.cs

lostmsu · 2020-08-02T05:19:44Z

src/runtime/typemanager.cs

+                MethodInfo[] methods = impl.GetMethods(tbFlags);
+                foreach (MethodInfo method in methods)
+                {
+                    string name = method.Name;
+                    if (!(name.StartsWith("tp_") ||
+                          name.StartsWith("nb_") ||
+                          name.StartsWith("sq_") ||
+                          name.StartsWith("mp_") ||
+                          name.StartsWith("bf_")
+                    ))
+                    {
+                        continue;
+                    }
+
+                    if (seen.Contains(name))
+                    {
+                        continue;
+                    }
+
+                    typeslots.Add(InitializeSlot(TypeSlots.getSlotNumber(name), method));
+                    seen.Add(name);


I vaguely remember seeing this code elsewhere. You should remove it from the original location.

@lostmsu its in this same file. I'll cleanup once I get things working

src/runtime/typemanager.cs

improve comments

koubaa · 2020-08-07T01:55:13Z

@filmor I don't think this alone will rid us of TypeOffset. tp_dict for instance does not appear to be covered in PyType_FromSpec and we use that offset in many places.

filmor · 2020-08-07T07:25:32Z

This is nevertheless a huge step forward. If we can minimise the offsets in active use, we can probably maintain the respective offset info with a small script. By the way, this PEP tracks more efforts that will help us in this regard: https://www.python.org/dev/peps/pep-0620/

koubaa · 2020-08-08T18:03:55Z

@filmor @lostmsu I'm getting an error that I am puzzled by.

When I run this code

import clr
import System
class Sub(System.Exception):
 pass

x=Sub()

or even replace the last line by
x = Sub.__new__(Sub)

It crashes inside of CPython with this message:

The last place in pythonnet when it reaches this point is in the CLRObject constructor at this line
IntPtr dict = Marshal.ReadIntPtr(py, ObjectOffset.TypeDictOffset(tp));

I've been debugging this for several hours now and I wish I could say I gained some more insight but I really haven't. What I did find is that the System.Exception MetaType construction seems to not go through the code path which I've modified.

Perhaps a more minor issue is this one - I am not sure if it is related:

>>> import System
__main__:1: DeprecationWarning: builtin type PropertyObject has no __module__ attribute
__main__:1: DeprecationWarning: builtin type EventObject has no __module__ attribute
__main__:1: DeprecationWarning: builtin type FieldObject has no __module__ attribute

koubaa · 2020-08-08T18:04:59Z

This is nevertheless a huge step forward. If we can minimise the offsets in active use, we can probably maintain the respective offset info with a small script. By the way, this PEP tracks more efforts that will help us in this regard: https://www.python.org/dev/peps/pep-0620/

@filmor I'll watch that PEP. It looks like its for 3.10 which isn't that far away

lostmsu · 2020-08-09T20:25:07Z

@koubaa the assert actually looks like it comes from .NET Debug.Assert call. Are you sure this is actually from CPython?

koubaa · 2020-08-09T21:28:40Z

@lostmsu yes I'm sure. I added Debug.Assert to try to better understand what's happening. In fact, the dialog which comes from the latter looks different.

lostmsu · 2020-09-10T21:50:31Z

@koubaa have you tried this code (Sub class) on master without your change? Does it work? If it does not, can you contribute a test (mark it disabled/inconclusive)? Then I can try to work on it outside of this PR.

The overload of CreateType you modified only works for metatypes defined in Python.Runtime (e.g. children of ManagedType). All other CLR types are handled via

pythonnet/src/runtime/typemanager.cs

Line 111 in 61d1b7c

internal static IntPtr CreateType(ManagedType impl, Type clrType)

and that includes System.Exception, System.Object and all derived types.

It is probably related to my shenanigans with a similar problem. You might want to ask a similar question on StackOverflow about the relationship between PyType_FromSpec and tp_basicsize. The documentation is surprisingly scarce.

I just checked on Win64 Python's reflection of System.Exception takes 80 bytes == ExceptionOffset.Size(), which is as expected. However, the Sub class takes 88 bytes, which is not. We need an explanation as to why Python adds extra bytes when deriving from System.Exception. Depending on it we should either change the Assert condition, or change our code to ensure Python does not need to add anything when deriving from System.Exception.

I just checked and the reflected System.Exception type has tp_dictoffset > 0, so that's not it.

amos402 · 2020-09-11T01:40:08Z

src/runtime/typemanager.cs

+        private static TypeSlots getSlotNumber(string methodName)
+        {
+            return (TypeSlots)Enum.Parse(typeof(TypeSlots), methodName);
+        }


I don't like to use string to get some constant information if it's not necessary, not to mention it use reflection for just getting the enum value.

@amos402 what do you propose?

Just use the enum value directly.

koubaa · 2020-09-11T02:28:28Z

@lostmsu Actually I discovered this issue from an existing test case: test_constructors.py test_subclass_constructor()

I thought I could do one overload at a time so it would be easier to bisect issues, do you think that it is wrong to expect this to work?

I'll look into tp_basicsize. I have recently been poking around in CPython source so I'll try to see if I can figure out what the relationship is.

lostmsu · 2020-09-11T02:55:26Z

The larger type object gets created for Sub here:

pythonnet/src/runtime/metatype.cs

Line 96 in 61d1b7c

IntPtr type = NativeCall.Call_3(func, tp, args, kw);

amos402 · 2020-09-11T03:09:03Z

We need an explanation as to why Python adds extra bytes when deriving from System.Exception

/* PyException_HEAD defines the initial segment of every exception class. */
#define PyException_HEAD PyObject_HEAD PyObject *dict;
PyObject *args; PyObject *traceback;
PyObject *context; PyObject *cause;
char suppress_context;

https://github.com/python/cpython/blob/4e02981de0952f54bf87967f8e10d169d6946b40/Include/pyerrors.h#L10-L14

lostmsu · 2020-09-11T03:15:47Z

@amos402 my understanding is that the class hierarchy looks like this: Sub -> System.Exception -> builtins.BaseException -> builtins.object. The question is why the first -> adds 8 bytes, not the last one. The last one works as intended.

I just checked on master, and the same assert actually fails there, so this is not caused by the @koubaa 's change in this PR, and can be evaluated separately.

koubaa · 2020-09-13T02:17:53Z

@lostmsu on master, its a pythonnet assertion (you can ignore the dialogs and the python interpreter is still up). With my branch, its a crash in native code. Perhaps @amos402 's fix will also fix things in my branch, I can try to cherry-pick the relevant changes interop.cs and see what happens

koubaa · 2020-09-13T02:56:15Z

@lostmsu @amos402 I confirm that with Amos's interop changes I do not have any crash anymore. I submitted a PR to land amos' change separate from his soft shutdown branch and have that commit cherry-picked in this branch so that I can continue to work on this.

koubaa · 2020-10-10T22:39:57Z

@lostmsu @amos402 I am looking at this again this weekend. I need time to understand the changes from soft-shutdown here...

lostmsu · 2020-11-26T06:35:00Z

@koubaa , if you are not actively working on this, I might pick it up over the holidays.

koubaa · 2020-11-28T01:36:58Z

@lostmsu no I'm not actively working on it anymore. I resolved some of the merge conflicts locally from the soft shutdown PR but haven't had much time to figure out how to get this working.

lostmsu · 2020-11-28T02:24:12Z

Actually, I also decided against it for now, as I don't have any idea how to clear slots for types creates with FromSpec, if we remove TypeOffset.

lostmsu · 2020-11-28T05:59:23Z

Closing for now, as this has been temporarily abandoned

Start to implement PyType_FromSpec type approach

27fb3c3

koubaa added 4 commits August 1, 2020 12:21

remove unused code

bd26014

always add null slot

81ba32c

change CreateType

44d364e

add comment

1eb7f32

lostmsu reviewed Aug 2, 2020

View reviewed changes

some progress & slight refactoring

5a2ccff

lostmsu reviewed Aug 2, 2020

View reviewed changes

src/runtime/typemanager.cs Outdated Show resolved Hide resolved

implement some PR suggestions

68b9d22

lostmsu reviewed Aug 3, 2020

View reviewed changes

src/runtime/typemanager.cs Outdated Show resolved Hide resolved

lostmsu reviewed Aug 3, 2020

View reviewed changes

src/runtime/typemanager.cs Outdated Show resolved Hide resolved

fix memory layout

71c20f2

koubaa commented Aug 4, 2020

View reviewed changes

src/runtime/typemanager.cs Outdated Show resolved Hide resolved

koubaa added 4 commits August 3, 2020 21:58

remove static constructor

5a8da7b

Use struct

3587638

push missed change

79d6a66

rename interop slots

0e74e0d

improve comments

re-add commented block

6333888

amos402 reviewed Sep 11, 2020

View reviewed changes

lostmsu mentioned this pull request Sep 11, 2020

Multiple assertion failures for Python classes derived from System.Exception in debug builds #1218

Closed

lostmsu closed this Nov 28, 2020

lostmsu mentioned this pull request Feb 22, 2021

PyType class, wrapper for Python types #1395

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start to implement PyType_FromSpec type approach #1196

Start to implement PyType_FromSpec type approach #1196

koubaa commented Aug 1, 2020 •

edited

Loading

koubaa commented Aug 1, 2020 •

edited

Loading

filmor commented Aug 1, 2020

lostmsu Aug 2, 2020

koubaa Aug 2, 2020

lostmsu Aug 2, 2020

koubaa Aug 2, 2020

lostmsu Aug 2, 2020

koubaa Aug 2, 2020

koubaa commented Aug 7, 2020

filmor commented Aug 7, 2020

koubaa commented Aug 8, 2020

koubaa commented Aug 8, 2020

lostmsu commented Aug 9, 2020

koubaa commented Aug 9, 2020 •

edited

Loading

lostmsu commented Sep 10, 2020

amos402 Sep 11, 2020

koubaa Sep 11, 2020

amos402 Sep 11, 2020

koubaa commented Sep 11, 2020

lostmsu commented Sep 11, 2020

amos402 commented Sep 11, 2020 •

edited

Loading

lostmsu commented Sep 11, 2020 •

edited

Loading

koubaa commented Sep 13, 2020

koubaa commented Sep 13, 2020

koubaa commented Oct 10, 2020

lostmsu commented Nov 26, 2020

koubaa commented Nov 28, 2020

lostmsu commented Nov 28, 2020

lostmsu commented Nov 28, 2020

Start to implement PyType_FromSpec type approach #1196

Start to implement PyType_FromSpec type approach #1196

Conversation

koubaa commented Aug 1, 2020 • edited Loading

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

koubaa commented Aug 1, 2020 • edited Loading

filmor commented Aug 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koubaa commented Aug 7, 2020

filmor commented Aug 7, 2020

koubaa commented Aug 8, 2020

koubaa commented Aug 8, 2020

lostmsu commented Aug 9, 2020

koubaa commented Aug 9, 2020 • edited Loading

lostmsu commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koubaa commented Sep 11, 2020

lostmsu commented Sep 11, 2020

amos402 commented Sep 11, 2020 • edited Loading

lostmsu commented Sep 11, 2020 • edited Loading

koubaa commented Sep 13, 2020

koubaa commented Sep 13, 2020

koubaa commented Oct 10, 2020

lostmsu commented Nov 26, 2020

koubaa commented Nov 28, 2020

lostmsu commented Nov 28, 2020

lostmsu commented Nov 28, 2020

koubaa commented Aug 1, 2020 •

edited

Loading

koubaa commented Aug 1, 2020 •

edited

Loading

koubaa commented Aug 9, 2020 •

edited

Loading

amos402 commented Sep 11, 2020 •

edited

Loading

lostmsu commented Sep 11, 2020 •

edited

Loading