Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

QuantConnect Update#1385

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

Closed
C-SELLERS wants to merge34 commits intopythonnet:masterfromC-SELLERS:refactor-qc-pythonnet3
Closed

QuantConnect Update#1385

C-SELLERS wants to merge34 commits intopythonnet:masterfromC-SELLERS:refactor-qc-pythonnet3

Conversation

C-SELLERS
Copy link

@C-SELLERSC-SELLERS commentedFeb 15, 2021
edited by filmor
Loading

What does this implement/fix? Explain your changes.

This PR seeks to bring QuantConnect/PythonNet 's changes to the root repo.

Since QuantConnect's fork was so distant from the root repo it required that I reapply all the needed changes in order to keep desired functionality. To denote this in the commits I referenced the PR I was trying to replicate. Please be aware that Github will link these to the root #n PR but they are all referencing QuantConnect/PythonNet PRs

Any other comments?

QuantConnect Changes Reflected (Reference QuantConnect/PythonNet for original PR):

Patches to address failures in Lean:

  • MethodBinder changes to allow implicit conversion. This one was particularly tricky and seemed impossible to patch together with the MethodBinder.cs in the root. Unfortunately it involved tearing apart a lot of it and rebuilding only what was needed. It may be possible to refactor this to look more like the root and still pass our tests. (58d5df0 ,44e089a ,108eacf ,6379568 ,9f2796a )
  • Revert PythonNet PRWrap returned objects in interface if method return type is interface #1240 ; this PR broke fetching interface derived classes data members that weren't defined in the interface. (ed6ab18 )
  • Converting PyObject to Int fix; this reflects the way PythonNet used to convert values to Int32, 64, unsigned etc, withPyNumber_Long but still keeps the use ofRuntime.PyLong_AsSignedSize_t to finish the conversion. The bug that discovered this was trying to convert a Python float to int, this used to work but was failing until this refactor. Reference the added test inbd94e49. (d87584b )

Tests added:

Some tests had to be adjusted for behavior changes caused by our commits. I have adjusted enough to have all PythonNet embedded tests passing, but there are still some issues with PyTests that need to be addressed/resolved.

I'm aware this ahuge collection of changes, so I don't expect this to be easy but would hope maybe there is a way to bring our branches together so we can help improve PythonNet in the future.

Also note this branch is also a pending PR to QuantConnect/PythonNet . Reference this PR atQuantConnect#47

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 toAUTHORS
  • Updated theCHANGELOG

C-SELLERSand others added30 commitsFebruary 15, 2021 13:13
@dnfadmin
Copy link

dnfadmin commentedFeb 15, 2021
edited
Loading

CLA assistant check
All CLA requirements met.

@C-SELLERSC-SELLERS marked this pull request as ready for reviewFebruary 16, 2021 00:23
@filmor
Copy link
Member

filmor commentedFeb 16, 2021
edited
Loading

Thank you for taking this up again.

I'm still very much against doing these conversions without a way to shut them off (while I agree that they might be good defaults). We nowadays have a system to implement additional conversions and configure them at runtime, codecs for the types you listed would be appreciated:https://github.com/pythonnet/pythonnet/wiki/Codecs:-customizing-object-marshalling-between-.NET-and-Python

This might also be implementable as a codec. I'll have a closer look

This looks nice, needs a separate PR.

Also looks like a codec candidate.

  • MethodBinder changes to allow implicit conversion. This one was particularly tricky and seemed impossible to patch together with the MethodBinder.cs in the root. Unfortunately it involved tearing apart a lot of it and rebuilding only what was needed. It may be possible to refactor this to look more like the root and still pass our tests. (58d5df0 ,44e089a ,108eacf ,6379568 ,9f2796a )

This would definitely need a separate PR. You could start with a PR that adds the tests.

Could you give a concrete example of what is failing now? Can it be mitigated using the__implementation__ member?

  • Converting PyObject to Int fix; this reflects the way PythonNet used to convert values to Int32, 64, unsigned etc, withPyNumber_Long but still keeps the use ofRuntime.PyLong_AsSignedSize_t to finish the conversion. The bug that discovered this was trying to convert a Python float to int, this used to work but was failing until this refactor. Reference the added test inbd94e49. (d87584b )

This looks good, can you create a PR for this one?

C-SELLERS reacted with thumbs up emoji

@koubaa
Copy link
Contributor

@filmor@C-SELLERS I sent an email to the pythonnet group about issues coming from#1240.@lostmsu suggested some things (here are 4 and 5 of his suggestions):

"
4. IronPython apparently allows explicit interface implementations to be called (instead of returning interface-wrapped instances), if there’s no conflict with regular methods. Perhaps we should consider this as an option.
5. Alternatively, we can keep wrapped instances, but allow to resolve public members too if the method is not found in the interface. I like this option better than 4, because actual type of an element in ISomething[] won’t change behavior of invoking interface member. It might still affecthasattr, but this is somewhat expected."

I think (4) is something like a revert.

C-SELLERS reacted with thumbs up emoji

@lostmsu
Copy link
Member

lostmsu commentedFeb 17, 2021
edited
Loading

Converting PyObject to Int fix; this reflects the way PythonNet used to convert values to Int32, 64, unsigned etc, with PyNumber_Long but still keeps the use of Runtime.PyLong_AsSignedSize_t to finish the conversion. The bug that discovered this was trying to convert a Python float to int, this used to work but was failing until this refactor. Reference the added test inbd94e49. (d87584b )

This looks good, can you create a PR for this one?

The old behavior (implicit conversions between numerical types like in the new test) should not be restored. I suggest you require explicit conversion viaint(decimal_value) on Python side or a similar method. Implicit conversion is dangerous (especially in finance).

C-SELLERS reacted with thumbs up emoji

@lostmsu
Copy link
Member

I'm still very much against doing these conversions without a way to shut them off (while I agree that they might be good defaults). We nowadays have a system to implement additional conversions and configure them at runtime, codecs for the types you listed would be appreciated:https://github.com/pythonnet/pythonnet/wiki/Codecs:-customizing-object-marshalling-between-.NET-and-Python

We also have a repository for community-contributed codecs:https://github.com/pythonnet/codecs

C-SELLERS reacted with thumbs up emoji

@C-SELLERS
Copy link
Author

Thank you guys for your feedback!

I plan to look into these when permitted and hope to start PR'ing the changes individually that can work! I definitely need to take some time and look at the codec setup you guys have, I wasn't quite sure how it worked and I didn't really understand the wiki either. I'll search for some examples and test it out and see if our conversions can be done that way.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@C-SELLERS@dnfadmin@filmor@koubaa@lostmsu@Martin-Molinero

[8]ページ先頭

©2009-2025 Movatter.jp