-
Notifications
You must be signed in to change notification settings - Fork 747
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
Add codecs for functions #1080
Conversation
src/embed_tests/Codecs.cs
Outdated
Assert.IsFalse(codec.CanDecode(fooFunc, typeof(bool))); | ||
|
||
//CanDecode does not work for variadic actions | ||
//Assert.IsFalse(codec.CanDecode(fooFunc, typeof(Action<object[]>))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary:
- I don't understand the purpose of
MethodWrapper2
as opposed to the currently supported ways to pass delegates/methods. - Ideally, this should work with any delegate types, which I think can be easily done with reflection.
src/embed_tests/Codecs.cs
Outdated
Assert.IsFalse(codec.CanDecode(fooFunc, typeof(bool))); | ||
|
||
//CanDecode does not work for variadic actions | ||
//Assert.IsFalse(codec.CanDecode(fooFunc, typeof(Action<object[]>))); |
There was a problem hiding this comment.
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)
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 So you only actually need to implement |
@lostmsu I need to write an embedding test to show what I want to do. |
There was a problem hiding this 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<*>
.
@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? 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(); 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. |
@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:
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? |
@koubaa do we have to? Can you achieve the same using public |
@lostmsu I don't see a way to increment a reference count using PyObject or to use Converter.ToManaged. |
@koubaa instead of incrementing refcount you can just keep the PyObject. |
@lostmsu I'll try it. |
@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 |
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.
AUTHORS
CHANGELOG