Skip to content

Add codecs for functions #1080

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

Closed
wants to merge 6 commits into from
Closed

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Mar 6, 2020

What does this implement/fix? Explain your changes.

...

Does this close any currently open issues?

This adds codecs to convert between python functions and actions. The tests which I added pass but I have some concerns about the code which I will comment about inline.

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

Assert.IsFalse(codec.CanDecode(fooFunc, typeof(bool)));

//CanDecode does not work for variadic actions
//Assert.IsFalse(codec.CanDecode(fooFunc, typeof(Action<object[]>)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lostmsu because of the interface of CanDecode this doesn't work. Since I defined functions in C# to use object[] I cannot determine the number of arguments without an instance.

Copy link
Member

Choose a reason for hiding this comment

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

You can create a special delegate type just for variadic parameter handling, and check for it explicitly in the codec.
E.g. delegate void VariadicAction<T>(params T[] args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lostmsu The issue is with the number of arguments, not the type of the arguments

Copy link
Contributor Author

@koubaa koubaa Mar 9, 2020

Choose a reason for hiding this comment

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

@lostmsu I think I get your point. Do you mean rather than Action<object[]> and Func<object[], object> I can simply use VariadicAction so if I have a situation like this:

C#

public int CallMe(Func<int, int, string, double, object> func) {
    return func(1,2, "hello", 4.4, new SomeObject());
}

Python

def call_me(i, j, name, real_value, obj):
    return 1

You expect that given typeof(func) you can convert it to VariadicAction and call Invoke on it dynamically? I can try and I think this may work except it may have a restriction to use the same type (preferably object) in all of the argument slots.

Copy link
Member

@lostmsu lostmsu left a comment

Choose a reason for hiding this comment

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

Summary:

  1. I don't understand the purpose of MethodWrapper2 as opposed to the currently supported ways to pass delegates/methods.
  2. Ideally, this should work with any delegate types, which I think can be easily done with reflection.

Assert.IsFalse(codec.CanDecode(fooFunc, typeof(bool)));

//CanDecode does not work for variadic actions
//Assert.IsFalse(codec.CanDecode(fooFunc, typeof(Action<object[]>)));
Copy link
Member

Choose a reason for hiding this comment

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

You can create a special delegate type just for variadic parameter handling, and check for it explicitly in the codec.
E.g. delegate void VariadicAction<T>(params T[] args)

@lostmsu
Copy link
Member

lostmsu commented Mar 6, 2020

Actually, I'd like to see a specific use case for this change. Either I don't understand something, or the only thing Python.NET does not support currently is passing Python callables to Action<*> parameters. Reverse works though.

So you only actually need to implement IPyObjectDecoder.

@koubaa
Copy link
Contributor Author

koubaa commented Mar 7, 2020

Actually, I'd like to see a specific use case for this change. Either I don't understand something, or the only thing Python.NET does not support currently is passing Python callables to Action<*> parameters. Reverse works though.

So you only actually need to implement IPyObjectDecoder.

@lostmsu I need to write an embedding test to show what I want to do.

Copy link
Member

@lostmsu lostmsu left a comment

Choose a reason for hiding this comment

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

You need to rewrite it using delegate reflection as I mentioned earlier, so it could work with any delegate types, not just Func<*> and Action<*>.

@koubaa
Copy link
Contributor Author

koubaa commented Mar 9, 2020

@lostmsu I found that getting the number of arguments in python using inspect signature is not reliable because in the CanDecode method the codec only has the PyObject_TYPE of the PyObject rather than the object itself. Are you open to changing the contract of the codecs to pass in the object itself and let the codec get its type?

image

I used delegate reflection to get the number of arguments from the C# delegate. But I don't see how that would help with binding to any delegate types. I need to construct a delegate of the type given by the parameter argument and then return it from the decoder. I guess I can use four delegate types:

delegate void UnaryActionDelegate();
delegate void VariadicActionDelegate(object[] args);
delegate object UnaryFuncDelegate();
delegate object VariadicFuncDelegate(object[] args);

which I can easily construct. But you cannot even construct a System.Action with a UnaryActionDelegate instance. So I am failing to see how the delegate reflection will help with supporting Action and Func.

That being said I think it makes sense to implement these delegates anyways so I can support delegate binding.

@koubaa koubaa mentioned this pull request Mar 11, 2020
4 tasks
@lostmsu lostmsu mentioned this pull request Mar 11, 2020
4 tasks
@koubaa
Copy link
Contributor Author

koubaa commented Mar 22, 2020

@lostmsu I am working on moving this codec to the codecs repo. While doing so I am encountering some build issues related to access levels in the core library:

  • Runtime.XIncref is marked internal
  • I can't use Converter.ToManaged or ToPython because the Converter class is internal

We can fix it by setting InternalsVisibleTo in Python.Runtime to the codecs assembly, but this will not fix things for folks (like myself) who may want to implement codecs in my own external libraries. Are there alternatives to these methods that are public and if not what options would I have?

@lostmsu
Copy link
Member

lostmsu commented Mar 22, 2020

@koubaa do we have to? Can you achieve the same using public PyObject class?

@koubaa
Copy link
Contributor Author

koubaa commented Mar 24, 2020

@lostmsu I don't see a way to increment a reference count using PyObject or to use Converter.ToManaged.

@lostmsu
Copy link
Member

lostmsu commented Mar 24, 2020

@koubaa instead of incrementing refcount you can just keep the PyObject. PyObject.As<T> is a direct counterpart of Converter.ToManaged.

@koubaa
Copy link
Contributor Author

koubaa commented Mar 24, 2020

@lostmsu I'll try it.

@koubaa
Copy link
Contributor Author

koubaa commented Mar 24, 2020

@lostmsu that was helpful. I can use As<> and even object.ToPython (a public extension method defined in python runtime). Testing will show if I get the reference count working, will update the codecs repo soon

@lostmsu lostmsu closed this May 14, 2020
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.

3 participants