Provide hook to implement repr #808

koubaa · 2019-01-20T02:40:38Z

Issue: 680

What does this implement/fix? Explain your changes.

This allows C# types to provide __repr__ implementations

Does this close any currently open issues?

680

Any other comments?

Please confirm my expections in the test I added. Inheritance hierarchies in particular were tricky to get right.

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

codecov · 2019-01-20T18:47:25Z

Codecov Report

Merging #808 into master will decrease coverage by 0.28%.
The diff coverage is 55.26%.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
- Coverage   76.91%   76.62%   -0.29%     
==========================================
  Files          64       64              
  Lines        5935     5973      +38     
  Branches      976      986      +10     
==========================================
+ Hits         4565     4577      +12     
- Misses       1040     1059      +19     
- Partials      330      337       +7

Flag	Coverage Δ
#setup_linux	`65.3% <ø> (ø)`	⬆️
#setup_windows	`75.85% <55.26%> (-0.29%)`	⬇️

Impacted Files	Coverage Δ
src/runtime/classbase.cs	`74.8% <55.17%> (-6.57%)`	⬇️
src/runtime/exceptions.cs	`79.03% <55.55%> (-1.84%)`	⬇️
src/runtime/delegatemanager.cs	`81.96% <0%> (-4.92%)`	⬇️
src/runtime/pythonexception.cs	`76.66% <0%> (-3.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 868cd52...82aea1f. Read the comment docs.

den-run-ai · 2019-01-21T01:28:53Z

@koubaa one error here:

=================================== ERRORS ====================================
___________________ ERROR collecting src/tests/test_repr.py ___________________
ImportError while importing test module 'C:\projects\pythonnet\src\tests\test_repr.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
src\tests\test_repr.py:7: in <module>
    from Python.Test import (ReprTest)
E   ImportError: cannot import name 'ReprTest' from 'Python.Test' (unknown location)
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!

koubaa · 2019-01-21T13:35:04Z

@denfromufa How do I run the tests locally? I want to make sure they work before pushing. (I just tested manually in the console and tried to follow the pattern)

koubaa · 2019-01-22T02:01:48Z

I figured it out (took longer than I'll admit!)

koubaa · 2019-01-22T02:49:14Z

One test fails because the repr of System.OverflowException went from
OverflowException('Simple message..."
to
<System.OverflowException object at 0xADDRESS>

I'll need to figure out how the old behavior was working to see why its broken now.

den-run-ai · 2019-01-22T20:25:22Z

@koubaa were you able to run the tests locally? If not, then I can search if we have instructions somewhere.

koubaa · 2019-01-23T00:43:44Z

@denfromufa yes I can run locally now. I have vs2017 installed and ran this to build:

python setup.py install --xplat

then this to test:

python -m pytest

Should this be in the README?

den-run-ai · 2019-01-23T02:05:48Z

This is only partial tests, there are also embedding tests run through nunit. Documentation improvements are welcome, probably this one should go into wiki.

…

On Tue, Jan 22, 2019, 6:43 PM Mohamed Koubaa ***@***.***> wrote: @denfromufa <https://github.com/denfromufa> yes I can run locally now. I have vs2017 installed and ran this to build: python setup.py install --xplat then this to test: python -m pytest Should this be in the README? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#808 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgZ5RkZgmBsugyNIIRfmhr349PPwytMks5vF7BCgaJpZM4aJb9z> .

koubaa · 2019-01-23T23:39:46Z

@denfromufa sounds good. I didn't see any embedded test fails in previous CI runs so I'm OK running these partial tests for now.

I tried to find where OverflowException repr is coming from (on master) and actually no breakpoints in tp_repr defined were hit. So I really don't know (1) how else to look for it and (2) how I could test that its an exception in classbase tp_repr and fallback to python defaults.

koubaa · 2019-02-04T23:14:32Z

@denfromufa I am back from a vacation so I looked at this again. I found this change which removed tp_repr from ExceptionClassObject. After some trial and error I discovered that __repr__() works on master because of what's done in SetArgsAndCause and the fact that a __repr__() was not defined. Now that ClassBase implements tp_repr, the CPython runtime will not fall back to the args/cause which were set.

My plan is to re-implement repr in ExceptionClassObject as was done before that change. Please let me know if you have any concerns with this approach.

koubaa · 2019-02-05T01:39:57Z

This is only partial tests, there are also embedding tests run through nunit. Documentation improvements are welcome, probably this one should go into wiki.

I just edited the wiki with what I know about so far.

koubaa · 2019-02-05T01:57:51Z

python tests now pass in CI. Embedded tests pass locally (on windows) but not in CI, so I'm adding some logging statements to see where.

filmor

Thanks already for your contribution. I have to think a bit about whether the "new" __str__ behaviour is how I would expect things to work.

src/runtime/classbase.cs

Issue: 680

koubaa · 2019-02-11T01:50:47Z

I'm having a lot of trouble with these CI tests.

The latest Travis job is not using my latest changes. Appveyor at least reports the correct SHA1 but have the below errors.
Appveyor 2.7 x64 xplat hangs for one hour when about to run embedded tests.
Appveyor 3.4 x86 xplat fails in pip install
Appveyor 3.4 x64 xplat also fails in pip install
Appveyor 2.7 x86 seems to have blown up because of the print statements I added. Is there a reason why Console.WriteLine is crashing (i.e. is stdout unavailable?) and if so what's the preferred way to add this sort of debugging logging statements?
Appveyor 2.7 x64 complains of a test failure but I cannot find much more details.
Appveyor 3.4 x86 also fails in setuptools
Appveyor 3.4 x64 also fails in setuptools.

Since all of the appveyor py3.4 are failing in setuptools it seems to me that its fallen out of maintenance. The 1 hour hangs concern me most since everything is queued and it delays other tests.

den-run-ai · 2019-02-21T17:42:17Z

@filmor we can probably make python 3.4 optional in CI tests as a temporary solution until we drop it?

filmor · 2019-02-25T16:14:00Z

I'll have a look into this this weekend. Could you please remove the debug output?

koubaa · 2019-02-27T22:54:39Z

done

koubaa · 2019-03-07T23:33:28Z

Seems better but its hard to determine what's wrong with x64 2.7. It says python tests failed but no indication which ones.

filmor · 2019-03-08T06:00:08Z

From the logs I think that the issue is aimply the casing of the ReprTests.cs. In the repository it's written in camel case, while in the csproj file ot's reprtests.cs. Correct the csproj and it should go through.

koubaa · 2019-03-08T23:51:53Z

@filmor done. Hopefully this works. Do you agree with the new str behavior()?

filmor · 2019-04-08T07:13:10Z

Each dot indicates a successful test, so you can just count ;)

I'll have a look into this as soon as I have finally finished the 2.4.0 release.

codecov-io · 2019-09-12T15:22:44Z

Codecov Report

Merging #808 into master will decrease coverage by 2.65%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
- Coverage   86.71%   84.05%   -2.66%     
==========================================
  Files           1        1              
  Lines         301      301              
==========================================
- Hits          261      253       -8     
- Misses         40       48       +8

Flag	Coverage Δ
#setup_linux	`65.44% <ø> (ø)`	⬆️
#setup_windows	`65.78% <ø> (-5.65%)`	⬇️

Impacted Files	Coverage Δ
setup.py	`84.05% <0%> (-2.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60e6045...264c78b. Read the comment docs.

filmor · 2019-09-12T19:31:45Z

Looks like the problems were magically fixed :)

lostmsu · 2019-09-12T21:34:25Z

src/runtime/classbase.cs

+                }
+
+                //If the object defines __repr__, call it.
+                System.Reflection.MethodInfo reprMethodInfo = instType.GetMethod("__repr__");


Since all .NET types have ToString, why does it even try __repr__? If somebody really needs it, they can do override string ToString() => __repr__()

That's not the same, though. ToString is more akin to __str__, there is no real equivalent to __repr__ in .NET.

Yeah, this is the tp_str implementation. tp_repr is below

@lostmsu See the comment:
//The return value must be a string object. If a class defines repr() but not str(),
//then repr() is also used when an “informal” string representation of instances of that
//class is required.

This sentence comes straight from the python manual, I didn't make it up

@koubaa the default inheritance hierarchy for C# classes from Python's point of view is Python's object <- System.Object <- CustomCSharpClass.

Since for all .NET classes overriding ToString is how you implement __str__, I don't see why it should be different for System.Object. Hence I think .NET classes should just always call ToString for tp_str.

@lostmsu I took a look. I agree some kind of consensus between interface vs reflection for dunder methods should be reached and applied throughout the code base. One argument for reflection is that you can use extension methods to add these. If I take an existing C# codebase and write some extension methods in a separate assembly, I can provide __repr__ and __getattr__ to it. What do you think?

@koubaa, extension methods would be hard to find. You'd have to enumerate all types in all assemblies. GetMethods won't return them.

In addition to that, what would be the semantics of multiple assemblies defining an extension method with the same signature?

@lostmsu ah, didn't know that.

@lostmsu I am still not convinced about the interface approach. It would require a hard build dependency on C# libraries onto PythonNet. It seems like a tough sell for some of the mature C# libraries out there.

@lostmsu I changed this as you requested. I don't think I can capture the repr function in a Func object since this is a static method.

src/runtime/classbase.cs

src/tests/test_repr.py

lostmsu · 2019-09-14T17:23:57Z

src/runtime/classbase.cs

+                }
+
+                //If the object defines __repr__, call it.
+                System.Reflection.MethodInfo reprMethodInfo = instType.GetMethod("__repr__");


Yeah, this is the tp_str implementation. tp_repr is below

koubaa

Looks like the problems were magically fixed :)

This is equal parts annoying and relieving

koubaa · 2019-09-15T18:16:22Z

src/runtime/classbase.cs

+                }
+
+                //If the object defines __repr__, call it.
+                System.Reflection.MethodInfo reprMethodInfo = instType.GetMethod("__repr__");


@lostmsu See the comment:
//The return value must be a string object. If a class defines repr() but not str(),
//then repr() is also used when an “informal” string representation of instances of that
//class is required.

This sentence comes straight from the python manual, I didn't make it up

src/runtime/classbase.cs

filmor · 2019-09-27T07:04:48Z

To help you in disagreeing, I propose the another style: We add a new generic abstract class (or interface with default methods when we convert to C#8) PythonWrapper and allow it to be registered for automatic conversion:

using SomeLib;

[Python.Conversion]
public class SomeClassWrapper : PythonWrapper<SomeClass> {
    public string Repr() => $"<SomeClass {this.wrapped.Name}>";
}

Now this means that we would need to link Python.Runtime.dll (or a much smaller DLL that is only used for these markers and the interface) into the wrapper DLL, but this is not an issue in embedding situations and everywhere else we could use the mentioned "smaller", platform-independent DLL.

lostmsu · 2019-09-27T17:23:53Z

@filmor not sure I understood your proposal.
Can you give a more complete example? What should PythonWrapper<T> do?

koubaa · 2019-10-17T22:24:26Z

Hey, can this go in yet?

filmor · 2019-10-18T08:08:45Z

It can, thanks a lot for your contribution :)

DanBarzilian · 2020-06-16T15:49:15Z

src/runtime/classbase.cs

+                Runtime.PyTuple_SetItem(args, 0, ob);
+                IntPtr reprFunc = Runtime.PyObject_GetAttrString(Runtime.PyBaseObjectType, "__repr__");
+                var output =  Runtime.PyObject_Call(reprFunc, args, IntPtr.Zero);
+                Runtime.XDecref(args);


Decreasing args reference count actually decreases ob reference count, which marks it as garbage for the GC!
After a while GC frees the object which causes the program to crash if the object is used again.
One fix for this can be adding the line:
Runtime.XIncref(ob);
at line 269 (before setting ob to the tuple's first argument).

Thanks a lot for catching this. Would you prepare a small PR?

Provide hook to implement __repr__

koubaa mentioned this pull request Jan 20, 2019

How to implement __repr__ for a C# class to be used in python.net? #680

Closed

koubaa force-pushed the repr branch 2 times, most recently from 859f80b to 8f6bdce Compare February 5, 2019 01:30

koubaa force-pushed the repr branch from 8f6bdce to 48e3077 Compare February 5, 2019 01:57

filmor added this to the 2.4.0 milestone Feb 5, 2019

filmor reviewed Feb 5, 2019

View reviewed changes

src/runtime/classbase.cs Outdated Show resolved Hide resolved

src/runtime/classbase.cs Outdated Show resolved Hide resolved

src/runtime/classbase.cs Outdated Show resolved Hide resolved

Provide hook to implement __repr__

351d9ff

Issue: 680

koubaa force-pushed the repr branch from e093acd to 107d22f Compare February 10, 2019 22:07

koubaa force-pushed the repr branch from e5fe9a2 to 351d9ff Compare February 27, 2019 22:54

Merge branch 'master' into repr

d71a391

fix casing

2a002d7

filmor modified the milestones: 2.4.0, 2.4.1 Apr 12, 2019

filmor added 3 commits June 20, 2019 19:32

Merge branch 'master' into repr

502f0c3

Merge branch 'master' into repr

eb4004a

Merge branch 'master' into repr

c6ae6a4

filmor added the next label Sep 12, 2019

lostmsu requested changes Sep 12, 2019

View reviewed changes

lostmsu requested changes Sep 14, 2019

View reviewed changes

koubaa added 2 commits September 15, 2019 13:32

update based on review

0916406

fix whitespace

2fb6a43

koubaa commented Sep 15, 2019

View reviewed changes

koubaa added 2 commits September 15, 2019 13:54

fix whitespace

9fc5b5f

fix

00b55db

amos402 reviewed Sep 24, 2019

View reviewed changes

src/runtime/classbase.cs Outdated Show resolved Hide resolved

koubaa added 3 commits October 16, 2019 17:56

Don't use repr in __str__. Clean up unneeded tests

accc797

fix leak

ff5d96c

Merge branch 'master' into repr

264c78b

lostmsu approved these changes Oct 17, 2019

View reviewed changes

filmor merged commit 4a9457f into pythonnet:master Oct 18, 2019

lostmsu modified the milestones: 2.4.1, 2.5.0 Apr 23, 2020

DanBarzilian reviewed Jun 16, 2020

View reviewed changes

AlexCatarino pushed a commit to QuantConnect/pythonnet that referenced this pull request Jun 27, 2020

Provide hook to implement __repr__ (pythonnet#808)

50ccbfd

Provide hook to implement __repr__

Provide hook to implement __repr__ #808

Provide hook to implement __repr__ #808

Conversation

koubaa commented Jan 20, 2019 • edited Loading

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

codecov bot commented Jan 20, 2019 • edited Loading

Codecov Report

den-run-ai commented Jan 21, 2019

koubaa commented Jan 21, 2019

koubaa commented Jan 22, 2019

koubaa commented Jan 22, 2019

den-run-ai commented Jan 22, 2019

koubaa commented Jan 23, 2019

den-run-ai commented Jan 23, 2019 via email

koubaa commented Jan 23, 2019

koubaa commented Feb 4, 2019 • edited Loading

koubaa commented Feb 5, 2019

koubaa commented Feb 5, 2019

filmor left a comment

Choose a reason for hiding this comment

koubaa commented Feb 11, 2019

den-run-ai commented Feb 21, 2019

filmor commented Feb 25, 2019

koubaa commented Feb 27, 2019

koubaa commented Mar 7, 2019

filmor commented Mar 8, 2019

koubaa commented Mar 8, 2019

filmor commented Apr 8, 2019

codecov-io commented Sep 12, 2019 • edited Loading

Codecov Report

filmor commented Sep 12, 2019

lostmsu Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koubaa Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koubaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmor commented Sep 27, 2019

lostmsu commented Sep 27, 2019

koubaa commented Oct 17, 2019

filmor commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Provide hook to implement repr #808

Provide hook to implement repr #808

koubaa commented Jan 20, 2019 •

edited

Loading

codecov bot commented Jan 20, 2019 •

edited

Loading

koubaa commented Feb 4, 2019 •

edited

Loading

codecov-io commented Sep 12, 2019 •

edited

Loading

lostmsu Sep 12, 2019 •

edited

Loading

koubaa Sep 16, 2019 •

edited

Loading