Enable user classes to override how Python.NET processes parameters of their functions #835

lostmsu · 2019-03-28T23:34:07Z

What does this implement/fix? Explain your changes.

This change introduces IPyArgumentConverter, that enables hooking into the
process of converting Python argument values into CLR types when invoking
.NET functions from Python. See an example on the new wiki page.

If a user class needs custom conversion for the parameters of its functions, it can specify
the desired implementation of IPyArgumentConverter using PyArgConverter attribute
either on the class/struct, or for the entire assembly.

Does this close any currently open issues?

This implements #823

Any other comments?

This is but one option on how to achieve #823 . I am concerned, that it does not let users specify the converter per instance, and does not allow specifying converter for 3rd-party classes.

This change has an additional performance overhead on the first Bind call on each instance of MethodBinder due to the need to look the attribute up.

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 (see an example)
Add yourself to AUTHORS
Updated the CHANGELOG

codecov · 2019-03-28T23:36:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@33db56d). Click here to learn what that means.
The diff coverage is 86.58%.

@@           Coverage Diff            @@
##             master    #835   +/-   ##
========================================
  Coverage          ?   65.2%           
========================================
  Files             ?      66           
  Lines             ?    5970           
  Branches          ?     994           
========================================
  Hits              ?    3893           
  Misses            ?    1719           
  Partials          ?     358

Flag	Coverage Δ
#setup_linux	`65.3% <ø> (?)`
#setup_windows	`64.43% <86.58%> (?)`

Impacted Files	Coverage Δ
src/runtime/pyargconverter.cs	`81.81% <81.81%> (ø)`
src/runtime/methodbinder.cs	`64.54% <87.09%> (ø)`
src/runtime/defaultpyargconverter.cs	`87.5% <87.5%> (ø)`

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 33db56d...8448fe6. Read the comment docs.

filmor

This is already a huge step forward, thank you very much :)

Regarding your concerns, I can't think of a case where one would want per-instance conversion. In that case I'd rather extend the interface to pass the instance object along s.t. the converter can branch out on its own.

Third party conversion should be quite simple to add, I commented on the respective section.

filmor · 2019-04-01T10:25:15Z

src/runtime/methodbinder.cs

 using System.Reflection;

 namespace Python.Runtime
 {
+    using System.Linq;


Move this to the top.

filmor · 2019-04-01T10:28:56Z

src/runtime/methodbinder.cs

+                    ?? method.DeclaringType?.Assembly
+                      .GetCustomAttributes(typeof(PyArgConverterAttribute), inherit: false)
+                      .OfType<PyArgConverterAttribute>()
+                      .SingleOrDefault();


Here you could additionally introduce a "global" (maybe per engine) lookup table (class -> converter) into which one could register the marshalers even if one can't modify the declaring assembly.

Did not want to overoptimize at first. Weren't there some issues with AppDomains for global caches?

Oh yes, there is plenty of brokenness in that general direction, that's why I put the "(maybe per engine)" there. We have other global data though already, and for the one usecase of multiple AppDomains that I know of (reloading code in Unity) it works because they take down the PythonEngine and restart it, s.t. always exactly one is running. The comment was also more of a "fyi", not a requirement for merging :)

src/runtime/pyargconverter.cs

lostmsu · 2019-04-17T21:55:46Z

The build is failing, but does not seem to be related to the changes in this PR.

filmor · 2019-04-23T13:25:41Z

I've retriggered the build and will have a look at your changes :)

lostmsu · 2019-05-08T19:28:10Z

@filmor I've discovered, that this does not entirely cover my own use case as-is. Perhaps it needs more refining before upstreaming.

The use case I have is .NET wrapper classes for Python classes. E.g. classes in .NET, that have interface, that copies some Python class, and a single PyObject field pointing to the actual Python object.

Problem I faced is that it is unclear what to do when a wrapper class is inherited.

// using default arg converter
class Wrapper {
   PyObject impl;
   public virtual void Overridable(CustomType o) => impl.overridable(o);
   public void NonOverridable(CustomType o) => impl.non_overridable(o);
}

[PyArgConverter(typeof(CustomArgConverter))]
class UserDerived: Wrapper {
  public override void Overridable(CustomType o) => o.DoSomething();
}

Now I send a wrapped instance of UserDerived to Python code. While it is clear, that if Python calls Overridable on it, the argument should be converted using CustomArgConverter, it is unclear what converter should be used in case Python calls NonOverridable.

In my use case, CustomArgConverter must be applied to NonOverridable too. However, the implementation in this PR will not do this, as NonOverridable.DeclaringType is Wrapper, which does not have PyArgConverter attribute.

I can also see an opposite case, when the base class actually relies on using a specific converter.

lostmsu · 2019-05-12T01:56:58Z

Nevermind, I figured it out. It works as is.

lostmsu · 2019-06-17T19:40:09Z

@filmor do not rush this change. After playing a bit with it, I think we might need to change inherit: false into inherit: true, and add a couple of tests. The point being that the custom converter should be applied to derived classes automatically (which would be a breaking change if made later on).

Also, looks like it reformats .csproj file, which is unnecessary.

filmor · 2019-06-17T19:58:37Z

Yeah, this one I saw as well. I just wanted to see how this fares test-wise against master :)

lostmsu · 2019-06-20T19:50:00Z

Hm, the .csproj change seems to be a result of fda9499

lostmsu · 2019-08-23T00:09:11Z

Alrightie. Caught a bug in the original implementation, caused by partial initialization of MethodBinder when MethodInfo to be invoked is explicitly passed to Bind. Should now be mergeable.

codecov-io · 2019-08-23T00:36:32Z

Codecov Report

Merging #835 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
+ Coverage   86.71%   86.75%   +0.04%     
==========================================
  Files           1        1              
  Lines         301      302       +1     
==========================================
+ Hits          261      262       +1     
  Misses         40       40

Flag	Coverage Δ
#setup_linux	`65.56% <100%> (+0.11%)`	⬆️
#setup_windows	`71.52% <100%> (+0.09%)`	⬆️

Impacted Files	Coverage Δ
setup.py	`86.75% <100%> (+0.04%)`	⬆️

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 aaafea8...3804438. Read the comment docs.

lostmsu · 2019-10-08T19:17:24Z

@filmor , any chance you could review this? It is ready for merging.

filmor · 2019-10-09T05:04:59Z

Could you rebase it on the current master? There was a change in the MethodBinder in between.

lostmsu · 2019-10-21T21:06:59Z

AppVeyor seems to have succeeded, but for some reason not being picked up: https://ci.appveyor.com/project/pythonnet/pythonnet/builds/28269629

@filmor rebased on master

lostmsu · 2019-10-24T20:38:20Z

@filmor review?

…f Python objects when calling .NET methods

…g PyArgConverter attribute

…nherited methods using an attribute on a derived class

…explicitly

lostmsu · 2020-02-27T17:45:56Z

Removed from 2.x, will review for 3.x. Might have been superseded by codecs #1022

lostmsu marked this pull request as ready for review March 28, 2019 23:39

filmor reviewed Apr 1, 2019

View reviewed changes

filmor added this to the 2.4.1 milestone Apr 12, 2019

lostmsu force-pushed the PR/PyArgConverter branch from 07c096e to cfc9336 Compare August 22, 2019 21:09

lostmsu requested a review from filmor August 23, 2019 00:38

lostmsu force-pushed the PR/PyArgConverter branch from 8126d2f to 80a9e66 Compare October 21, 2019 19:10

filmor added the next label Nov 22, 2019

lostmsu mentioned this pull request Jan 10, 2020

is "Customizing interop" supported? #1030

Closed

lostmsu added 8 commits January 31, 2020 12:03

introduced IPyArgumentConverter interface, that controls marshaling o…

c355bd6

…f Python objects when calling .NET methods

enabled an ability to override Python to .NET argument coversion usin…

b5cae6c

…g PyArgConverter attribute

mentioned PyArgConverter in the CHANGELOG.md

bea9e0d

fixed pyargconverter.cs not included into the older project file

11a3ee6

moved default arg converter to its own file; cached converter lookup

9c81661

fixed defaultpyargconverter.cs not included in the older project file

b6421e9

when looking for a custom argument converter look at base types too

6cd7bd4

a test to ensure it is possible to override argument conversion for i…

56ca4c9

…nherited methods using an attribute on a derived class

lostmsu added 2 commits January 31, 2020 12:03

fixed MarshallerOverride test

7f894fa

fixed crash in MethodBinder.Bind when MethodInfo to invoke is passed …

aaafea8

…explicitly

lostmsu force-pushed the PR/PyArgConverter branch from b7eeb06 to aaafea8 Compare January 31, 2020 20:04

Merge branch 'master' into PR/PyArgConverter

3804438

lostmsu removed this from the 2.4.1 milestone Feb 27, 2020

lostmsu closed this Mar 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable user classes to override how Python.NET processes parameters of their functions #835

Enable user classes to override how Python.NET processes parameters of their functions #835

lostmsu commented Mar 28, 2019

codecov bot commented Mar 28, 2019 •

edited

Loading

filmor left a comment

filmor Apr 1, 2019

filmor Apr 1, 2019

lostmsu Apr 1, 2019

filmor Apr 3, 2019

lostmsu commented Apr 17, 2019

filmor commented Apr 23, 2019

lostmsu commented May 8, 2019

lostmsu commented May 12, 2019

lostmsu commented Jun 17, 2019 •

edited

Loading

filmor commented Jun 17, 2019

lostmsu commented Jun 20, 2019

lostmsu commented Aug 23, 2019

codecov-io commented Aug 23, 2019 •

edited

Loading

lostmsu commented Oct 8, 2019

filmor commented Oct 9, 2019

lostmsu commented Oct 21, 2019

lostmsu commented Oct 24, 2019

lostmsu commented Feb 27, 2020

Enable user classes to override how Python.NET processes parameters of their functions #835

Enable user classes to override how Python.NET processes parameters of their functions #835

Conversation

lostmsu commented Mar 28, 2019

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

codecov bot commented Mar 28, 2019 • edited Loading

Codecov Report

filmor left a comment

Choose a reason for hiding this comment

filmor Apr 1, 2019

Choose a reason for hiding this comment

filmor Apr 1, 2019

Choose a reason for hiding this comment

lostmsu Apr 1, 2019

Choose a reason for hiding this comment

filmor Apr 3, 2019

Choose a reason for hiding this comment

lostmsu commented Apr 17, 2019

filmor commented Apr 23, 2019

lostmsu commented May 8, 2019

lostmsu commented May 12, 2019

lostmsu commented Jun 17, 2019 • edited Loading

filmor commented Jun 17, 2019

lostmsu commented Jun 20, 2019

lostmsu commented Aug 23, 2019

codecov-io commented Aug 23, 2019 • edited Loading

Codecov Report

lostmsu commented Oct 8, 2019

filmor commented Oct 9, 2019

lostmsu commented Oct 21, 2019

lostmsu commented Oct 24, 2019

lostmsu commented Feb 27, 2020

codecov bot commented Mar 28, 2019 •

edited

Loading

lostmsu commented Jun 17, 2019 •

edited

Loading

codecov-io commented Aug 23, 2019 •

edited

Loading