NotificationsYou must be signed in to change notification settings
Fork750
Star5.1k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

lostmsu wants to merge11 commits intopythonnet:masterfromlosttech:PR/PyArgConverter

Closed

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

lostmsu wants to merge11 commits intopythonnet:masterfromlosttech:PR/PyArgConverter

Conversation

Copy link

Member

lostmsu commentedMar 28, 2019

What does this implement/fix? Explain your changes.

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

If a user class needs custom conversion for the parameters of its functions, it can specify
the desired implementation ofIPyArgumentConverter usingPyArgConverter 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 firstBind call on each instance ofMethodBinder 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 (seean example)
Add yourself toAUTHORS
Updated theCHANGELOG

Copy link

codecovbot commentedMar 28, 2019•
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@33db56d).Click here to learn what that means.
The diff coverage is86.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 byCodecov. Last update33db56d...8448fe6. Read thecomment docs.

lostmsu marked this pull request as ready for review

March 28, 2019 23:39

filmor reviewed

Apr 1, 2019

View reviewed changes

Copy link

Member

filmor left a comment

Choose a reason for hiding this comment

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

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.

src/runtime/methodbinder.cs Outdated

		using System.Reflection;

		namespace Python.Runtime
		{
		using System.Linq;

Copy link

Member

filmorApr 1, 2019

Choose a reason for hiding this comment

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

Move this to the top.

src/runtime/methodbinder.cs Outdated

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

Copy link

Member

filmorApr 1, 2019

Choose a reason for hiding this comment

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

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.

Copy link

MemberAuthor

lostmsuApr 1, 2019

Choose a reason for hiding this comment

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

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

Copy link

Member

filmorApr 3, 2019

Choose a reason for hiding this comment

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

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 OutdatedShow resolvedHide resolved

filmor added this to the2.4.1 milestone

Apr 12, 2019

Copy link

MemberAuthor

lostmsu commentedApr 17, 2019

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

Copy link

Member

filmor commentedApr 23, 2019

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

Copy link

MemberAuthor

lostmsu commentedMay 8, 2019

@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 singlePyObject 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 converterclassWrapper{PyObjectimpl;publicvirtualvoidOverridable(CustomTypeo)=>impl.overridable(o);publicvoidNonOverridable(CustomTypeo)=>impl.non_overridable(o);}[PyArgConverter(typeof(CustomArgConverter))]classUserDerived:Wrapper{publicoverridevoidOverridable(CustomTypeo)=>o.DoSomething();}

Now I send a wrapped instance ofUserDerived to Python code. While it is clear, that if Python callsOverridable on it, the argument should be converted usingCustomArgConverter, it is unclear what converter should be used in case Python callsNonOverridable.

In my use case,CustomArgConverter must be applied toNonOverridable too. However, the implementation in this PR will not do this, asNonOverridable.DeclaringType isWrapper, which does not havePyArgConverter attribute.

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

Copy link

MemberAuthor

lostmsu commentedMay 12, 2019

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

Copy link

MemberAuthor

lostmsu commentedJun 17, 2019•
edited
Loading

@filmor do not rush this change. After playing a bit with it, I think we might need to changeinherit: false intoinherit: 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.

Copy link

Member

filmor commentedJun 17, 2019

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

Copy link

MemberAuthor

lostmsu commentedJun 20, 2019

Hm, the .csproj change seems to be a result offda9499

lostmsu force-pushed thePR/PyArgConverter branch from07c096e tocfc9336Compare

August 22, 2019 21:09

Copy link

MemberAuthor

lostmsu commentedAug 23, 2019

Alrightie. Caught a bug in the original implementation, caused by partial initialization ofMethodBinder whenMethodInfo to be invoked is explicitly passed toBind. Should now be mergeable.

Copy link

codecov-io commentedAug 23, 2019•
edited
Loading

Codecov Report

Merging#835 intomaster willincrease coverage by0.04%.
The diff coverage is100%.

@@            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 byCodecov. Last updateaaafea8...3804438. Read thecomment docs.

lostmsu requested a review fromfilmor

August 23, 2019 00:38

Copy link

MemberAuthor

lostmsu commentedOct 8, 2019

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

Copy link

Member

filmor commentedOct 9, 2019

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

lostmsu force-pushed thePR/PyArgConverter branch from8126d2f to80a9e66Compare

October 21, 2019 19:10

Copy link

MemberAuthor

lostmsu commentedOct 21, 2019

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

Copy link

MemberAuthor

lostmsu commentedOct 24, 2019

@filmor review?

filmor added the next label

Nov 22, 2019

lostmsu mentioned this pull request

Jan 10, 2020

is "Customizing interop" supported?#1030

Closed

lostmsu added8 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 added2 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 thePR/PyArgConverter branch fromb7eeb06 toaaafea8Compare

January 31, 2020 20:04

Merge branch 'master' into PR/PyArgConverter

3804438

lostmsu removed this from the2.4.1 milestone

Feb 27, 2020

Copy link

MemberAuthor

lostmsu commentedFeb 27, 2020

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

lostmsu closed this

Mar 10, 2020

Labels

None yet

3 participants

Movatterモバイル変換

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

Uh oh!

Conversation

lostmsu commentedMar 28, 2019

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

codecovbot commentedMar 28, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

filmor left a comment

Choose a reason for hiding this comment

Uh oh!

filmorApr 1, 2019

Choose a reason for hiding this comment

Uh oh!

filmorApr 1, 2019

Choose a reason for hiding this comment

Uh oh!

lostmsuApr 1, 2019

Choose a reason for hiding this comment

Uh oh!

filmorApr 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lostmsu commentedApr 17, 2019

Uh oh!

filmor commentedApr 23, 2019

Uh oh!

lostmsu commentedMay 8, 2019

Uh oh!

lostmsu commentedMay 12, 2019

Uh oh!

lostmsu commentedJun 17, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

filmor commentedJun 17, 2019

Uh oh!

lostmsu commentedJun 20, 2019

Uh oh!

lostmsu commentedAug 23, 2019

Uh oh!

codecov-io commentedAug 23, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lostmsu commentedOct 8, 2019

Uh oh!

filmor commentedOct 9, 2019

Uh oh!

lostmsu commentedOct 21, 2019

Uh oh!

lostmsu commentedOct 24, 2019

Uh oh!

lostmsu commentedFeb 27, 2020

Uh oh!

Uh oh!

codecovbot commentedMar 28, 2019•
edited
Loading

lostmsu commentedJun 17, 2019•
edited
Loading

codecov-io commentedAug 23, 2019•
edited
Loading