[3/3] Improve Signature and method table printing

PhilipVinc commented 8 months ago

very very WIP. I would also like to experiment with rich to print more coloured, more readable stuff, but this is already more readable...

In [1]: f.methods
Out[1]: 
Method List with # 7 methods:
 [0] f(x: str)
        <function f at 0x103a63060> @ /Users/filippo.vicentini/Dropbox/Ricerca/Codes/Python/plum/prova.py:6
 [1] f(x: int)
        <function f at 0x103c811c0> @ /Users/filippo.vicentini/Dropbox/Ricerca/Codes/Python/plum/prova.py:36
 [2] f(x: numbers.Number)
        <function f at 0x103c80ea0> @ /Users/filippo.vicentini/Dropbox/Ricerca/Codes/Python/plum/prova.py:16
 [3] f(x: numbers.Number, k: float, *, kk, j, ksa, **povolo)
        <function f at 0x103c80fe0> @ /Users/filippo.vicentini/Dropbox/Ricerca/Codes/Python/plum/prova.py:28
 [4] f(x: float, y: numbers.Number) -> int
        precedence=10
        <function f at 0x103c81080> @ /Users/filippo.vicentini/Dropbox/Ricerca/Codes/Python/plum/prova.py:24
 [5] f(x: numbers.Number, y: float, *z: tuple[int, ...])
        <function f at 0x103c81120> @ /Users/filippo.vicentini/Dropbox/Ricerca/Codes/Python/plum/prova.py:32
 [6] f(x: int, y: int, *z: tuple[int, ...])
        <function f at 0x103c811c0> @ /Users/filippo.vicentini/Dropbox/Ricerca/Codes/Python/plum/prova.py:36

It refactors a bit the internals. Need to find a better way to do it..

wesselb commented 8 months ago

Ah, that looks beautiful! Love the idea. :)

Let me know when you'd like me to review the PR or parts of it.

PhilipVinc commented 8 months ago

I would love some insight on how to compute the distance between (invalid) signatures!

wesselb commented 8 months ago

Ah, let me reply here instead of in the other issue. :)

Hmmm, I'm not exactly sure how a solution should look like, but maybe something simple like this could work:

def signature_distance(candidate: Signature, reference: Signature):
    distance = 0
    for t1, t2 in zip(candidate.types, reference.types):
        if not issubclass(t1, t2):
            distance += 1
    return distance

This isn't yet quite right, since it doesn't appropriately consider the number of arguments (len(candidate.types)) and varags.

Do you have any thoughts or ideas? The above is just the first thing that comes to mind. We could likely do something better.

One approach would be to proceed with something simple like this and later improve the distance function.

PhilipVinc commented 8 months ago

https://github.com/beartype/plum/assets/2407108/ee2ff29b-f08d-44b8-9485-ec3a0dae88c4

Ok, more updates... What do you think about this?

I think the main thing I would like to do is to replace the values printed in the resolution error with the types, at least for those that are not parametric... but that's not easily done...

Because it's hard to understand if the values printed there match or are related to the types below. maybe not hard, but not straightforward

wesselb commented 8 months ago

Ok, more updates... What do you think about this?

I absolutely love it!! That looks so good. This is awesome work! :)

I think the main thing I would like to do is to replace the values printed in the resolution error with the types,

This would be really nice, but seems like it would require a type_of function. I agree with you that it would make it easier to understand the list of closest methods.

PhilipVinc commented 8 months ago

@wesselb Ok, I cleaned up the PR.

This is now based on top of #112 .

It should be possible to implement this logic without the changes of #112 , so if you don't like those changes I could try to remove them from the PR, but in my opinion they modularise a bit the internals of plum (and It made it simpler for me to understand what I was working with.)

The major missing things from this PR is now:

ANSI detection: Printing colors/bolds/hyperlinks in terminal is all nice and great because most terminals support them. Theroretically one should check that the terminal supports ANSI codes (which is what is used to display colours in terminals) but we could relatively safely assume they are supported. However...
- When one uses output redirection, for example when running a batch job on a cluster, you don't want to print them because they show up as noise in the logs. Detecting this reliably is... a mess. See for example how complicated it is to detect if one is running on Jupyter (which is not a terminal).
- To do this... the most maintainable approach would be to add a dependency, rich, which is a no-dependency library for pretty-printing stuff, and use it. The library is solid (pip uses it as well since a year or so). Then, rich automatically detects and disables colors when they should not be
- Alternatively we could roll a simplified version of this, which would surely support fewer edge cases and be more prone to breaking with the benefit of not having the dependency.

I would frankly go for rich, but what do you think?

coveralls commented 8 months ago

Pull Request Test Coverage Report for Build 7596314753

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report. To ensure accuracy in future PRs, please see these guidelines. A quick fix for this PR: rebase it; your next report should be accurate.

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.8%) to 97.834%

Totals
Change from base Build 6552015703:	0.8%
Covered Lines:	1310
Relevant Lines:	1339

💛 - Coveralls

PhilipVinc commented 8 months ago

@wesselb I don't want to push, but in case you like/accept the approach and the few internal refactors, I can work my way to add tests and increase coverage again. (I don't want to do the work if you don't like the structure as it would be wasted effort).

wesselb commented 8 months ago

@PhilipVinc sorry for the delay! Yes, I absolutely love this!! It would be awesome to merge this. :) I think the changes in #112 are sensible too. I'll review those first, and then we can merge this.

With regards to rich, since it's a no-dependency library which is incredibly popular and well battle tested, I'd be happy to add it as a dependency. Another option would be to use rich if it is installed and otherwise print without colour. I see you've got a fairly concise custom implementation already in repr.py, so that would work too.

I'm slightly leaning towards just adding rich as a dependency for the reasons that you mentioned. What do you think?

PhilipVinc commented 7 months ago

@wesselb The PR is done and the tests have been updated, so I think it is time for a review. Sorry for taking all your time!

As you agreed to include rich, that's what I used, because it makes output over dumb terminals/mpi/output pipes clean and is a bit easier to use than alternatives.

Most functionality necessary for converting to displayable objects are in plum/repr.py with a lot of docstring. An important function there is rich_reprwhich is a decorator for classes to make them use rich repr handling.

Other minor changes are...

Resolution error printing is now delegated to the error types themselves instead of the Resolver. This simplifies the logic handling and removes the need for that _enhance_error function in Function.
Added a MethodList which is a thin wrapper over list that pretty prints methods.
Added some logic to detect which arguments are a mismatch and which are not, in order to colorise in red only those that mismatch.

Overall... this logic is ugly as hell, but I don't think it's possible to simplify those repr methods much..

wesselb commented 6 months ago

Hey @PhilipVinc! This PR has been waiting for a long time now. I'm very sorry about that. :( I sometimes struggle to find the time for this.

I will prioritise this PR next week!

PhilipVinc commented 6 months ago

bumpety bump

wesselb commented 4 months ago

@PhilipVinc I might add a few more tests, and then I think this is ready to be merged! Are you happy to merge it after a few more tests?

PhilipVinc commented 4 months ago

I think that your last change of method.py , in MethodList, should render the method itself, not the mismatch...

wesselb commented 4 months ago

@PhilipVinc How does this look to you? Ready to merge?

wesselb commented 4 months ago

@PhilipVinc Actually, you've indicated multiple times that you're ready to have this merged, so I'm gonna go ahead to merge this and tag a new release. :)

beartype / plum