- Notifications
You must be signed in to change notification settings - Fork3
add function codec#1
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lostmsu commentedApr 9, 2020
@koubaa I've added Python installation to CI. Please, rebase your branch. |
koubaa commentedApr 9, 2020
lostmsu commentedApr 9, 2020
@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]publicclassLocalTestConfig{[OneTimeSetUp]publicvoidAssemblySetup(){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 commentedApr 9, 2020
Hm, build still fails to find the library. Let me try to fix it. |
koubaa commentedApr 9, 2020
@lostmsu I can try what you did locally, but ideally this info can be in the wiki |
koubaa commentedApr 9, 2020
@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 commentedApr 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
lostmsu commentedApr 13, 2020
@koubaa you have to manually call |
koubaa commentedApr 14, 2020
@lostmsu done. tests fail due to the issue with the return value of the Func |
koubaa commentedApr 23, 2020
@lostmsu these changes are ready and tests pass locally. I don't know what happened to the CI: |
lostmsu left a comment
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.
I can not take this PR as is for two connected reasons:
- It is not generic enough. E.g. it should really support any type of delegate via reflection on
Invoke(like you did withGetNumArgs). - It reserves unintuitive meaning for standard C# delegate types, especially
Func<object[], object>. One should expect, that a parameter of typeFunc<object[], object>should take a Python function, that has a single argument of typelistor 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.
delegatevoidVariadicAction(object[]args);
or even
delegatevoidVariadicAction(PyObject[]args);
and use them instead of the standard types for the scenario in 2.
koubaa commentedMay 11, 2020
@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 commentedMay 11, 2020
@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 commentedMay 11, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@koubaa BTW, we can take this if you target it to custom delegate types instead of delegateobjectPythonFunction(object[]args);voidFoo(PythonFunctionpyFunc){Func<...,object>sharpFunc=(a,b,...)=>pyFunc(newobject[]{a,b, ...});// use sharpFunc normally} In fact, you don't even need a But I strongly suggest to try generalize this using |
koubaa commentedMay 12, 2020
@lostmsu I'll keep it my private repo then. I don't know how to accomplish what you're looking for |
Uh oh!
There was an error while loading.Please reload this page.
@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.