| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@filmor FYI. Clearly I need to fix runtime issues, do some cleanup and move things around but this is my general approach. |
Sorry, something went wrong.
|
Thanks for working on this! This could simplify compatibility with different Python versions a lot 👍 |
Sorry, something went wrong.
| 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 |
There was a problem hiding this comment.
Ideally, these extra types should go into their own files.
Sorry, something went wrong.
There was a problem hiding this comment.
@lostmsu I intend to do it after I get things working as part of the final cleanup
Sorry, something went wrong.
|
|
||
| public static IntPtr AllocPyTypeSpec(string typename, int obSize, int obFlags, IntPtr slotsPtr) | ||
| { | ||
| byte[] ascii = System.Text.Encoding.ASCII.GetBytes(typename); |
There was a problem hiding this comment.
.NET supports Unicode type names. Not sure about Python here.
Sorry, something went wrong.
There was a problem hiding this comment.
@lostmsu I think its possible. Maybe we should use UTF8 here - I should add a test to see for sure
Sorry, something went wrong.
| 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); |
There was a problem hiding this comment.
I vaguely remember seeing this code elsewhere. You should remove it from the original location.
Sorry, something went wrong.
There was a problem hiding this comment.
@lostmsu its in this same file. I'll cleanup once I get things working
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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/ |
Sorry, something went wrong.
|
@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 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 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 |
Sorry, something went wrong.
@filmor I'll watch that PEP. It looks like its for 3.10 which isn't that far away |
Sorry, something went wrong.
|
@koubaa the assert actually looks like it comes from .NET Debug.Assert call. Are you sure this is actually from CPython? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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 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. |
Sorry, something went wrong.
| private static TypeSlots getSlotNumber(string methodName) | ||
| { | ||
| return (TypeSlots)Enum.Parse(typeof(TypeSlots), methodName); | ||
| } |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
@amos402 what do you propose?
Sorry, something went wrong.
There was a problem hiding this comment.
Just use the enum value directly.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
The larger type object gets created for Sub here: pythonnet/src/runtime/metatype.cs Line 96 in 61d1b7c |
Sorry, something went wrong.
/* PyException_HEAD defines the initial segment of every exception class. */ |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@koubaa , if you are not actively working on this, I might pick it up over the holidays. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Closing for now, as this has been temporarily abandoned |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.