Skip to content

Codecs: customize set of Py<->C# conversions via PyObjectConversions #1022

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Feb 26, 2020

Conversation

lostmsu
Copy link
Member

@lostmsu lostmsu commented Dec 19, 2019

What does this implement/fix? Explain your changes.

This enables registering new automatic Py->C# and C#->Py conversions (codecs), that will run when Python calls .NET code and vice versa.

Does this close any currently open issues?

This is a new feature

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: see TupleCodec
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@lostmsu lostmsu force-pushed the PR/Codecs branch 2 times, most recently from e840a4b to 3dff8b5 Compare December 20, 2019 02:20
@lostmsu lostmsu mentioned this pull request Dec 20, 2019
4 tasks
@@ -23,7 +23,7 @@
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
<PythonBuildDir Condition="'$(TargetFramework)'=='net40' AND '$(PythonBuildDir)' == ''">$(SolutionDir)\bin\</PythonBuildDir>
<PublishDir Condition="'$(TargetFramework)'!='net40'">$(OutputPath)\$(TargetFramework)_publish</PublishDir>
<LangVersion>6</LangVersion>
<LangVersion>7.3</LangVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? @lostmsu

Copy link
Member Author

@lostmsu lostmsu Dec 22, 2019

Choose a reason for hiding this comment

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

Yes, I think I am using a few features. Is there a C# compiler, that does not support 7.3 yet?
It is 7.3 in Runtime for a few months now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lostmsu just wanted to make sure it wasn't added by mistake.

/// <summary>
/// Increase Python's ref counter for the given object, and get the object back.
/// </summary>
internal static IntPtr SelfIncRef(IntPtr op)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is neat and might simplify other code in the project

@koubaa
Copy link
Contributor

koubaa commented Jan 15, 2020

I like the conciseness of the terminology (encode/decode) but for me it isn't entirely obvious which direction (py <-> C#) is encoding and decoding. It is ok with me as long as we document it well enough.

Which other encoders should exist inside of Python.Runtime aside from the Tuple one you've added? Would you be OK with the List and Action ones I am interested in eventually being distributed by PythonNet?

I am writing the below snippet to document how it'd be available for external codecs to see how we like the syntax. To me it is not too bad:

import clr
clr.AddReference("MyCodecs")
import MyCodecs
for encoder in MyCodecs.Encoders:
    Python.Runtime.PyObjectConversions.RegisterEncoder(encoder)
for decoder in MyCodecs.Decoders:
    Python.Runtime.PyObjectConversions.RegisterDecoder(decoder)

@@ -135,8 +135,16 @@ internal static IntPtr ToPython(object value, Type type)
return result;
}

if (value is IList && !(value is INotifyPropertyChanged) && value.GetType().IsGenericType)
{
if (Type.GetTypeCode(type) == TypeCode.Object && value.GetType() != typeof(object)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

glad this comes before other builtin conversions but ultimately the built-in conversions should probably use this codec system internally.

@lostmsu lostmsu requested a review from filmor January 16, 2020 19:05
@lostmsu
Copy link
Member Author

lostmsu commented Jan 16, 2020

Pushed a fix to failing test


public bool CanEncode(Type type)
=> type.Namespace == typeof(TTuple).Namespace && type.Name.StartsWith(typeof(TTuple).Name + '`')
|| type == typeof(object) || type == typeof(TTuple);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe write this one out as a normal function block, it's a bit tedious to read.

Copy link
Member

Choose a reason for hiding this comment

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

Also, can't you just use type.IsGenericType && type.GetGenericTypeDefinition() == typeof(TTuple) or something like that?

@lostmsu
Copy link
Member Author

lostmsu commented Jan 30, 2020

Mark methods as obsolete until they are stable.

@codecov-io
Copy link

codecov-io commented Feb 1, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1022   +/-   ##
=======================================
  Coverage   86.75%   86.75%           
=======================================
  Files           1        1           
  Lines         302      302           
=======================================
  Hits          262      262           
  Misses         40       40
Flag Coverage Δ
#setup_linux 65.56% <ø> (ø) ⬆️
#setup_windows 71.52% <ø> (ø) ⬆️

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 5619fb9...41de69d. Read the comment docs.

@lostmsu
Copy link
Member Author

lostmsu commented Feb 13, 2020

@filmor I think this can be merged

@lostmsu
Copy link
Member Author

lostmsu commented Feb 26, 2020

Either I got lucky in this run, or #1050 actually was the real cause for test failures in this branch, and so was fixed by #1062.

@lostmsu lostmsu merged commit 4d1442d into pythonnet:master Feb 26, 2020
@amos402
Copy link
Member

amos402 commented Feb 28, 2020

The code style of this PR is totally different by the current code, is it really good for this?

@igiona
Copy link

igiona commented Sep 16, 2021

The issue #823 has been marked as solved by this PR.
It's not clear to me which approach has been taken here (from the ones described in the mentioned bug)?
@lostmsu is the PR going to make Python.NET fully support .NET objects providing the IDynamicMetaObjectProvider interface? I'm moving away from IronPython (which supports IDynamicMetaObjectProvider objects) and running into many issues...

From what I can see, this new feature didn't make it to the latest release, is there a plan for for a 2.5.3 or there will be only the v3.0.0?
Thanks

@lostmsu
Copy link
Member Author

lostmsu commented Sep 16, 2021

@igiona There's no support for IDynamicMetaObjectProvider on custom .NET objects. Instead for custom types to be presented to Python (or vice versa) as something they are not, you need to implement encoders and/or decoders: customizing object marshalling between .NET and Python

@igiona
Copy link

igiona commented Sep 17, 2021

Thank you @lostmsu for the quick answer.
This implies that the PythonNet assemblies needs to imported in the .NET application, which is not always possible/desirable (not technically, but rather "politically"/by-design.

What's the rational behind the decision to not support IDynamicMetaObjectProvider ?
Is there some architectural constraint that prevent this, or it has a chance to get supported by Python.NET in the future?

@lostmsu
Copy link
Member Author

lostmsu commented Sep 17, 2021

The rational is that pythonnet is an open source project, and nobody stepped up to implement this feature.

I am not sure what you mean by "PythonNet assemblies needs to imported in the .NET application, which is not always possible/desirable". You can develop and register codecs from Python (though development in Python might be tricky). You can also develop codecs in C#, compile them to a DLL, and register them from it. e.g.

clr.AddReference('MyCodecs.dll')

from MyCodecs import MyMegaCodecForTypeX

codec = MyMegaCodecForTypeX()
PyObjectConversions.RegisterEncoder(codec)
PyObjectConversions.RegisterDecoder(codec)

... work with TypeX ...

@igiona
Copy link

igiona commented Sep 19, 2021

The rational is that pythonnet is an open source project, and nobody stepped up to implement this feature.

This makes sense 👍

I did not checked yet the implementation above into details, but I assumed that the codecs have to implement the interfaces IPyObjectDecoder and/or IPyObjectEncoder in order to be able to be registered via RegisterEncoder/RegisterDecoder.
Which means that my C#-application (which provides these codec) needs to import the PaythonNET assemblies as dependency in order to compile ("which is not always possible/desirable"). Unless your implementation looks for the interface method names by reflection and wraps/maps them to the PythonNET interfaces internally!?

@lostmsu
Copy link
Member Author

lostmsu commented Sep 19, 2021

Again, the app does not have to import pythonnet assemblies. Codecs can be put into a separate library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants