Add python buffer api support by thesn10 · Pull Request #980 · pythonnet/pythonnet · GitHub

What does this implement/fix? Explain your changes.

Passing data like images from managed to python will be easyer than ever!
I implemented the python buffer api which makes copying big chunks of data directly into the python object buffer possible!

You can copy strings to python object:

PyObject array = scope.Eval("bytearray(3)");
byte[] managedArr = new UTF8Encoding().GetBytes(new char[] { 'a', 'b', 'c' });

PyBuffer buf = array.GetBuffer(out bool success, 0);
if (success)
{
    buf.WriteToBuffer(managedArr, 0, managedArr.Length);
    buf.Release();
}

Or copy images to python byte arrays:

byte[] imageArr = LoadImage();
PyObject array = scope.Eval("bytearray(" + managedArr.Length + ")");

PyBuffer buf = array.GetBuffer(out bool success, 0);
if (success)
{
    buf.WriteToBuffer(managedArr, 0, managedArr.Length);
    buf.Release();
}

Does this close any currently open issues?

No

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

Thank you very much, this is a nice addition!

I've added a few general comments and will look into the Python-API details of this tomorrow.

By the way, the failing CI is probably since you didn't add PyBuffer.cs to the old project files.

Fixed most of your comments. But i dont know why git marked almost all lines of the old csproject file as changed, even though i only added on line. I hope this is no problem :)

You probably changed the line endings

Codecov Report

Merging #980 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #980   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files           1        1           
  Lines         301      301           
=======================================
  Hits          261      261           
  Misses         40       40

Flag	Coverage Δ
#setup_linux	65.44% <ø> (ø)	⬆️
#setup_windows	71.42% <ø> (ø)	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1463538...3edb8f5. Read the comment docs.

This PR needs unit tests with good coverage. Currently, basic functionality is non-functional.

Why return success/failure via out, and not simply throw an exception?

... in which case you also have to release resources.

Your right. My bad.

This is the only line where _view is initialized, however, at this moment the memory _handle points to is empty (e.g. all zeroes). So the _view is also always empty.
None of the ifs below ever succeed.

Your wrong. _view should be empty, because PyObject_GetBuffer fills _view with data. Every if below works as expected

@Sngmng you might be using some funky runtime, which makes this happen. PyObject_GetBuffer does not get a pointer to _view. It gets a pointer to the rawData instead. The call to Marshal.PtrToStructure simply creates a copy of memory it is pointing to.

Oh my bad i messed up the order of these two lines when i was moving the code into the constructor. The PR was working before this commit tho.

Creating PyObject does not increment reference counter of the object. You have to manually do it in this getter.

But PyObject_GetBuffer() does. And PyBuffer_Release decrements the reference count... Just read the docs first. https://docs.python.org/3/c-api/buffer.html

But in this case you actually get a new reference, so you have to increase the recount, or cache the instance of pyobject in constructor.

Why am i getting a new refference? Isnt PyObject() just wrapping the handle that was already increfed? Whatever, im just going to add a XIncref as you say.

@Sngmng you are right, that it usually takes a pre-increfed pointer. But each instance requires its own incref. Otherwise when the instance is disposed, all other instances of PyObject pointing to the same reference will simply crash on access.

It would be good to reset these handles to zero, and check, that the buffer has not been released yet.

Py_size_t is not the same as Int64 aka long. There is an ongoing discussion about sizes of Python types.

Well it works with long and i didnt see a reason to use Py_ssize_t if it will get casted to long anyway. I looked it up in the python source and it litteraly typdefs Py_ssize_t as long. What do you recommend to do?

I don't know yet, as we did not work it out. C's long is not the same as C#'s long. The later is always Int64.

This code won't work on 32 bit systems. There C's long is almost always == Int32

@lostmsu What about something like this:

[StructLayout(LayoutKind.Sequential)]
internal struct Py_ssize_t
{
    [MarshalAs(UnmanagedType.SysInt)]
    private IntPtr size;

    public long GetValue()
    {
        if (Is32Bit)
            return size.ToInt32();
        else
            return size.ToInt64();
    }
}

@Sngmng this struct might not work either, because on 64-bit windows IntPtr is 64-bit, but Py_ssize_t is probably 32-bit. https://stackoverflow.com/questions/384502/what-is-the-bit-size-of-long-on-64-bit-windows

And that problem will have to be solved, before PR is accepted.

long and thus Py_ssize_t is 4 bytes on Windows (32 and 64 bit) and 8 bytes on other 64 bit systems. Very annoying.

See my comment below, I made this mistake (assuming Py_ssize_t == long) before ;)

Py_ssize_t is a typedef for intptr_t or ssize_t (on POSIX), so in all relevant cases it is equivalent in size to IntPtr.

This PR needs unit tests with good coverage. Currently, basic functionality is non-functional.

Yeah it needs unit tests. But why are you claiming its non functional? Did you even test this PR? It works as expected. Just try out one of the examples i provided, you will see.

Apart from that, im going to implement all of your other requested changes like implementing IDisposable later this day

Ive implemented most of the requested changes, but i dont know what you want me to do with the Py_ssize_t fields? Leave them as longs? Or use a struct like i proposed?

@Sngmng unfortunately, the only way I can think of so far is to have two struct types defined, and decide which one to use at runtime.

It is a sad state of things, I know.

BTW, there appears to be a number of warnings regarding XML doc comments on the new file in the build, I guess related to the recent refactoring: https://ci.appveyor.com/project/pythonnet/pythonnet/builds/28472200/job/3otegb2hqnq9f6on

InvalidOperationException?

Sorry for the confusion, we had this discussion already before (in #531) and the conclusion is that Py_ssize_t is indeed IntPtr (not long, though!), so if you change the longs in the Py_buffer definition and ensure that the copy is done correctly this is fine. (Currently you do the copy to Strides etc. assuming you have 64bit data).

@filmor it might be fine for passing function parameters which will occupy an entire register anyway, but for structs incorrect size will cause the following fields to be incorrectly placed, leading to wrong memory being accessed.

If our tests are run on Windows x64 (which we should do exactly for this kind of issue) they will likely catch the problem.

Hmm, it still seems to not quite work on x86, I'll try to reproduce this locally.

@filmor weird, should not be a problem on x86.

@filmor it might be fine for passing function parameters which will occupy an entire register anyway, but for structs incorrect size will cause the following fields to be incorrectly placed, leading to wrong memory being accessed.

If our tests are run on Windows x64 (which we should do exactly for this kind of issue) they will likely catch the problem.

I'm not sure what exactly this one was the answer to, but IntPtr is the correct type (on both 32 and 64 bit systems) and should lead to the correct layout.

Interestingly enough, non-xplat built Py3.7 ran through on x86...

@filmor you are correct. @Sngmng needs to investigate why non-xplat x86 builds fail.

Well, i tested Python 3.6 x86 + non-xplat. No errors at all.

Since i cant spot any error in the failing appveyor logs at all and my own test allways succeeds, Im beyond my capabilities. Maybe there is some config differences in the appveryor builds that cause the error? Maybe it randomly detects unmanaged memory copys and fails? I dont know man...

Yeah, it doesn't really fail but times out. I'll try to reproduce it locally (running exactly what AppVeyor is supposed to run), but I don't use Windows as my primary dev platform, so I need a while to set everything up.

@lostmsu Could you rereview already?

Should the message say "SizeFromFormat ..." not "GetPointer ..."?

Since this is a public method, why not throw a PythonException on failure?

Same, why not throw?

And here

_readonly should be bool

This cast to int should be checked to avoid data loss

Also, I think it is wrong to silently write _view.len instead of count, when count is > _view.len.
Return the number of bytes actually written, or throw an exception.

This cast to int should be checked to avoid data loss

@Sngmng what's your solution architecture? VS has unexpected behavior when project and solution don't match. Also try running tests from command line like CI does it, e.g. with nunit3-console.exe

@lostmsu I ran it through nunit3-console and it timeouts at the exact same line as appveyor. Which is odd because the tests didnt even run...
The output is not very much and doesnt give any error.

PS D:\Users\user\Downloads\gitrepos\pythonnetsn\src\embed_tests\bin> nunit3-console Python.EmbeddingTest.dll --trace=Verbose
NUnit Console Runner 3.10.0 (.NET 2.0)
Copyright (c) 2019 Charlie Poole, Rob Prouse
Samstag, 2. November 2019 00:00:32

Runtime Environment
   OS Version: Microsoft Windows NT 10.0.18362.0
  CLR Version: 4.0.30319.42000

Test Files
    Python.EmbeddingTest.dll

=> Python.EmbeddingTest.TestDomainReload.DomainReloadAndGC
[Program.Main] === creating domain for assembly test1, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
[Program.Main] Before Domain Unload on test1, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
[Program.Main] After Domain Unload on test1, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
[Program.Main] The Proxy object is not valid anymore, domain unload complete.
[Program.Main] === creating domain for assembly test2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
[Program.Main] Before Domain Unload on test2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
[Program.Main] After Domain Unload on test2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
[Program.Main] The Proxy object is not valid anymore, domain unload complete.

@Sngmng you can try --inprocess parameter, with and without debugger. E.g. https://docs.microsoft.com/en-us/visualstudio/debugger/how-to-debug-from-a-dll-project?view=vs-2019

@Sngmng you can try --inprocess parameter, with and without debugger. E.g. https://docs.microsoft.com/en-us/visualstudio/debugger/how-to-debug-from-a-dll-project?view=vs-2019

If i try --inprocess:

NUnit.Engine.NUnitEngineException : Cannot run tests in process - a 32 bit process is required.

--NUnitEngineException
Cannot run tests in process - a 32 bit process is required.
   bei NUnit.Engine.Runners.MasterTestRunner.InitializePackage()
   bei NUnit.Engine.TestEngine.GetRunner(TestPackage package)
   bei NUnit.ConsoleRunner.ConsoleRunner.RunTests(TestPackage package, TestFilter filter)

@Sngmng build fails because of the wrong casing in Py_buffer.cs in csproj file

@Sngmng you can try https://stackoverflow.com/questions/1507268/force-x86-clr-on-an-any-cpu-net-assembly corflags /32bit+ <PathToExe> to force nunit-console to run as 32 bit process (note, this is persisted).

If the binary is not AnyCPU, you could try building a x86 binary.

Looks good to me, as soon as tests pass in CI.
Maybe update CHANGELOG?

This is originally named FORMAT and not FORMATS but for some reason i get a "name not cls compilant" warning if i use FORMAT

@Sngmng have you tried tests locally after removing finalizer? Do they pass?

@Sngmng have you tried tests locally after removing finalizer? Do they pass?

Visual studio fooled me that they pass but they didnt when i retested through nunit3-console. Weird.

@Sngmng a sanity check: if you comment your tests out, do the tests pass?

@lostmsu
Sometimes errors seem weird but the answer couldnt be more obvious.
So i was comparing my unit tests with the others and noticed that mine was the only one using a PyScope. And then it hit me: If i used a PyScope, did i call Py.GIL() first?! Well it turned out i didnt.
Im surprised it took me a week to notice this obvious cause.😑

@Sngmng hahaha, now I remember facing the same issue. Damn. Looks like we should add GIL checks to many entry points...

@lostmsu yeah would be better hahaha. But the tests seem to fail again... seems like Py.GIL wasnt the only cause.. 😭

At least the failure is now less erratic in that it fails exactly for all Python 3 versions on Windows running on x86 :)

They should not be running in parallel. The test failure looks like it happens because of python engine reinitialization. @amos402 can you take a look at this PR to see if anything here could have corrupted memory on domain reinitialization? I don't see anything, but maybe you'll notice some issue.

@Sngmng , try removing finalizer on the buffer class. If that helps, we may have to take in PR without it, and reintroduce it later.

I'm not sure about that, maybe the cause is calling PyBuffer_Release in ~PyBuffer, it have not been got GIL before calling PyBuffer_Release(destructor called in GC thread). Also the destructor may call after Py_Finalize.

btw. I wonder why the PyBuffer not making an implementation of System.Stream?

A Stream implementation is (I think) out of scope here, but the GIL comment is valid.

@Sngmng can you rebase/merge master and update the PyBuffer finalizer to behave similarly to the one in PyObject (see latest changes in master)?

@Sngmng can you rebase/merge master and update the PyBuffer finalizer to behave similarly to the one in PyObject (see latest changes in master)?

yeah sure

All CLA requirements met.

Why is appveyor configured to do the conda build? I mean it fails everytime and is ignored anyway? And it takes 6min which is a lot. Removing these checks would improve the test speed to 1min which is a 6 times speed improvement. I am waiting multiple hours for appveyor because it is so slow, just because of the unnecessary conda build...

Related: #1063

@Sngmng the issue was just fixed in master by filmor.

Given 2.5 is just around the corner, and in 3.0 I'd like to fix #980 , which might affect the interfaces proposed here, I suggest we get it after 2.5 is released (should be soon).

@Sngmng the issues you mentioned are fixed in master, you should be able to update the finalizer now.

Be aware that the last commit to update the finalizer is still untested because i had some problems using nunit and couldnt run the tests.

Codecov Report

❗ No coverage uploaded for pull request base (master@49a230b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #980   +/-   ##
=========================================
  Coverage          ?   86.66%           
=========================================
  Files             ?        1           
  Lines             ?      300           
  Branches          ?        0           
=========================================
  Hits              ?      260           
  Misses            ?       40           
  Partials          ?        0

Flag	Coverage Δ
#setup_linux	65.33% <ø> (?)
#setup_windows	72.00% <ø> (?)

Impacted Files	Coverage Δ
setup.py	86.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49a230b...530e8c7. Read the comment docs.

@Sngmng oh, your Dispose method was perfectly fine. It is the finalizer (e.g. ~PyBuffer()), that needed to be updated.

Somehow this is already in o-O

filmor reviewed Oct 27, 2019

View reviewed changes

Comment thread src/runtime/pybuffer.cs Outdated

Comment thread src/runtime/pybuffer.cs Outdated

Comment thread src/runtime/pybuffer.cs Outdated

Comment thread src/runtime/pybuffer.cs Outdated

Comment thread src/runtime/pyobject.cs Outdated

Comment thread src/runtime/runtime.cs Outdated

lostmsu requested changes Oct 29, 2019

View reviewed changes

lostmsu reviewed Oct 29, 2019

View reviewed changes

filmor reviewed Oct 30, 2019

View reviewed changes

lostmsu reviewed Nov 1, 2019

View reviewed changes

lostmsu reviewed Nov 2, 2019

View reviewed changes

Comment thread src/runtime/pybuffer.cs Outdated

Comment thread src/runtime/pybuffer.cs Outdated

lostmsu approved these changes Nov 2, 2019

View reviewed changes

thesn10 commented Nov 2, 2019

View reviewed changes

lostmsu mentioned this pull request Nov 13, 2019

RFC: Use AdvancedDLSupport instead of P/Invoke or GetFunctionPointerForDelegate #987

Closed

Add Python buffer api support

3c9f7b3

filmor force-pushed the pybuffer2 branch from 5911800 to 3c9f7b3 Compare May 17, 2020 09:43

lostmsu added this to the 3.0.0 milestone May 22, 2020

Update finalizer

52cc24c

thesn10 and others added 2 commits May 29, 2020 08:25

Update finalizer 2

530e8c7

Merge branch 'master' into pybuffer2

abf957f

lostmsu closed this Oct 30, 2020

+                          byte[] rawData = new byte[size];
+                          _gchandle = GCHandle.Alloc(rawData, GCHandleType.Pinned);
+                          _handle = _gchandle.AddrOfPinnedObject();
+                          _view = (Runtime.Py_buffer)Marshal.PtrToStructure(_handle, typeof(Runtime.Py_buffer));

+                          public IntPtr buf;
+                          public IntPtr obj;        /* owned reference */
+                          public long len;
+                          public long itemsize;  /* This is Py_ssize_t so it can be

+                          if (disposedValue)
+                              throw new ObjectDisposedException("PyBuffer");
+                          if (ReadOnly)
+                              throw new Exception("Buffer is read-only");

+                      /// Copy contiguous len bytes from buf to view. fort can be 'C' or 'F' (for C-style or Fortran-style ordering). 0 is returned on success, -1 on error.
+                      /// </summary>
+                      /// <returns>0 is returned on success, -1 on error.</returns>
+                      public int FromContiguous(IntPtr buf, long len, char fort)

+                              throw new ObjectDisposedException(nameof(PyBuffer));
+                          if (Runtime.pyversionnumber < 36)
+                              throw new NotSupportedException("ToContiguous requires at least Python 3.6");
+                          return Runtime.PyBuffer_ToContiguous(buf, _handle, _view.len, order);

+                          }
+                      }
+                      public PyObject Object => new PyObject(_view.obj);

+                      public void Release()
+                      {
+                          Runtime.PyBuffer_Release(_handle);
+                          _gchandle.Free();

+                      public static long SizeFromFormat(string format)
+                      {
+                          if (Runtime.pyversionnumber < 39)
+                              throw new NotSupportedException("GetPointer requires at least Python 3.9");

+                      /// If this function is used as part of a getbufferproc, exporter MUST be set to the exporting object and flags must be passed unmodified.Otherwise, exporter MUST be NULL.
+                      /// </remarks>
+                      /// <returns>On success, set view->obj to a new reference to exporter and return 0. Otherwise, raise PyExc_BufferError, set view->obj to NULL and return -1;</returns>
+                      public int FillInfo(IntPtr exporter, IntPtr buf, long len, int _readonly, int flags)

+                          if (_view.ndim != 1)
+                              throw new NotSupportedException("Multidimensional arrays, scalars and objects without a buffer are not supported.");
+                          int copylen = count < (int)_view.len ? count : (int)_view.len;

+                      /// <summary>
+                      /// Controls the <see cref="PyBuffer.Format"/> field. If set, this field MUST be filled in correctly. Otherwise, this field MUST be NULL.
+                      /// </summary>
+                      FORMATS = 0x0004,

Add python buffer api support by thesn10 · Pull Request #980 · pythonnet/pythonnet · GitHub

Navigation Menu

Uh oh!

Add python buffer api support - #980

Add python buffer api support#980thesn10 wants to merge 4 commits intopythonnet:masterpythonnet/pythonnet:masterfrom thesn10:pybuffer2thesn10/pythonnet:pybuffer2Copy head branch name to clipboard

Conversation

thesn10 commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

filmor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filmor commented Oct 27, 2019

Uh oh!

thesn10 commented Oct 28, 2019

Uh oh!

filmor commented Oct 28, 2019

Uh oh!

codecov-io commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lostmsu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesn10 Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add python buffer api support#980
thesn10 wants to merge 4 commits into
pythonnet:masterfrom
thesn10:pybuffer2

thesn10 commented Oct 27, 2019 •
edited
Loading

codecov-io commented Oct 28, 2019 •
edited
Loading

lostmsu left a comment •
edited
Loading

lostmsu Oct 29, 2019 •
edited
Loading

thesn10 Oct 29, 2019 •
edited
Loading

thesn10 commented Oct 29, 2019 •
edited
Loading

lostmsu commented Oct 29, 2019 •
edited
Loading

lostmsu commented Oct 30, 2019 •
edited
Loading