Skip to content

add function codec#1

Closed
koubaa wants to merge 3 commits into
pythonnet:masterfrom
koubaa:function-codec
Closed

add function codec#1
koubaa wants to merge 3 commits into
pythonnet:masterfrom
koubaa:function-codec

Conversation

@koubaa

@koubaa koubaa commented Mar 29, 2020

Copy link
Copy Markdown

@lostmsu While running the test I get the exception "Could not load python37.dll". I wanted to push this first to get a preliminary review while I figure that out. It also appears that vs2019 is needed (for .net core 3.1). I don't mind but wasn't sure if this was intentional.

@lostmsu

lostmsu commented Apr 9, 2020

Copy link
Copy Markdown
Member

@koubaa I've added Python installation to CI. Please, rebase your branch.

@koubaa

koubaa commented Apr 9, 2020

Copy link
Copy Markdown
Author

@koubaa I've added Python installation to CI. Please, rebase your branch.

@lostmsu done. Although I suppose this won't fix the issue testing it locally.

@lostmsu

lostmsu commented Apr 9, 2020

Copy link
Copy Markdown
Member

@koubaa Locally, you can go to project settings and set PATH environment variable in the Debug section. I personally just have a setup class in the test project, that I never commit:

[SetUpFixture]
public class LocalTestConfig {
    [OneTimeSetUp]
    public void AssemblySetup()
    {
        Environment.SetEnvironmentVariable("PATH",
            @"C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python37_64\;"
            + Environment.GetEnvironmentVariable("PATH"));

        PythonEngine.PythonHome =
            @"C:\Users\USER\AppData\Local\Programs\Python\Python37";
    }
}

@lostmsu

lostmsu commented Apr 9, 2020

Copy link
Copy Markdown
Member

Hm, build still fails to find the library. Let me try to fix it.

@koubaa

koubaa commented Apr 9, 2020

Copy link
Copy Markdown
Author

@lostmsu I can try what you did locally, but ideally this info can be in the wiki

@koubaa

koubaa commented Apr 9, 2020

Copy link
Copy Markdown
Author

@lostmsu AssemblySetup didn't work, I debugged and my TestCallBacks SetUp function is called before AssemblySetup. I think the order NUnit runs these is not deterministic and since you can run individual tests from the test explorer I don't think It'll work in general even if it does run that setup first. I'll see if I can figure out how the pythonnet embedding tests sets up the python environment.

@dnfclas

dnfclas commented Apr 12, 2020

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@lostmsu

lostmsu commented Apr 13, 2020

Copy link
Copy Markdown
Member

@koubaa you have to manually call TestsRuntimeConfig.Ensure(); in [SetUp] for your tests.

@koubaa

koubaa commented Apr 14, 2020

Copy link
Copy Markdown
Author

@lostmsu done. tests fail due to the issue with the return value of the Func

@koubaa koubaa mentioned this pull request Apr 17, 2020
4 tasks
@koubaa

koubaa commented Apr 23, 2020

Copy link
Copy Markdown
Author

@lostmsu these changes are ready and tests pass locally. I don't know what happened to the CI:
https://github.com/pythonnet/codecs/actions/runs/85878219

@lostmsu lostmsu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can not take this PR as is for two connected reasons:

  1. It is not generic enough. E.g. it should really support any type of delegate via reflection on Invoke (like you did with GetNumArgs).
  2. It reserves unintuitive meaning for standard C# delegate types, especially Func<object[], object>. One should expect, that a parameter of type Func<object[], object> should take a Python function, that has a single argument of type list or C# array, not an arbitrary Python function, that would get objects from the array as individual arguments.

If you have a very specific use case that this fullfills, I suggest to simply keep this codec in your private codebase. Otherwise, it really needs to be properly generalized.

As for the special handling I mentioned in 2., I recommend to create special delegate types, e.g.

delegate void VariadicAction(object[] args);

or even

delegate void VariadicAction(PyObject[] args);

and use them instead of the standard types for the scenario in 2.

@koubaa

koubaa commented May 11, 2020

Copy link
Copy Markdown
Author

@lostmsu I thought you said these were the reasons you couldn't take it in the pythonnet repo but you could take it in the codecs repo?

@lostmsu

lostmsu commented May 11, 2020

Copy link
Copy Markdown
Member

@koubaa the reason not to take in the main repo is that conversion is lossy. E.g. decode followed by encode do not give an identical (or at least identically behaving) object back.

I think genericity is not really necessary on its own, but "unintuitive meaning for standard C# delegate types" is a real problem. I just think making a generic solution is easy enough vs going the route of defining custom function types to circumvent "unintuitive meaning for standard C# delegate types", and forcing everyone to use them with this codec.

@lostmsu

lostmsu commented May 11, 2020

Copy link
Copy Markdown
Member

@koubaa BTW, we can take this if you target it to custom delegate types instead of Func<object[], object> and other standard ones to avoid confusion. It should be enough for most use cases, users could then just have lambdas forwarding the custom delegate type to whatever type they need. E.g.

delegate object PythonFunction(object[] args);

void Foo(PythonFunction pyFunc) {
  Func<..., object> sharpFunc = (a,b,...) => pyFunc(new object[]{ a, b, ...});
  // use sharpFunc normally
}

In fact, you don't even need a PythonAction, because no return Python functions still return None.

But I strongly suggest to try generalize this using Reflection and Reflection.Emit/LINQ expressions.

@koubaa

koubaa commented May 12, 2020

Copy link
Copy Markdown
Author

@lostmsu I'll keep it my private repo then. I don't know how to accomplish what you're looking for

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