Skip to content

cached_property issue / descriptor issue #1833

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
ep12 opened this issue Apr 2, 2020 · 5 comments · Fixed by #1835
Closed

cached_property issue / descriptor issue #1833

ep12 opened this issue Apr 2, 2020 · 5 comments · Fixed by #1835
Labels
C-compat A discrepancy between RustPython and CPython

Comments

@ep12
Copy link

ep12 commented Apr 2, 2020

Feature

I have a class like this:

from functools import cached_property
def f(v):
    # do something
    ...

class C:
    def __init__(self, data: dict):
        self.data = data
    
    @cached_property
    def calculated_data(self) -> dict:
         return {k: f(v) for k, v in self.data.items()}

(For the exact implementation see ep12/PyOPM, but the implementation is not really important in this case.)
Something like

c = C(some_dict)
for k, v in c.calculated_data.items():
    # do something
    ...

results in an error:

Traceback (most recent call last):
  File "basic.py", line 17, in <module>
    m = p.match({0, 1, 2})  # not a dict -> m is None
  File "/home/simon/git/PyOPM/pyopm/core.py", line 263, in match
    for kt, v in self.compiled_pattern.items():
  File "/home/simon/git/RustPython/Lib/functools.py", line 951, in __get__
    "Cannot use cached_property instance without calling __set_name__ on it.")
TypeError: Cannot use cached_property instance without calling __set_name__ on it.

I think it might be related to the descriptor implementation, but __set_name__ does not seem to be called using rustpython. Everything works fine with cpython and pypy.
Since the cached_property implementation of rustpython and cpython is exactly the same, this might be an internal issue. I think that cached_property.__set_name__ should be called with name=func.__name__ (which is 'calculated_data', of course)... I think python itself performs this call (I could not find a line of python where __set_name__ gets called), so maybe the rustpython implementation does not do that at the moment.

I haven't looked at the rustpython source code (my rust skills are extremely limited) but maybe someone else knows where to have a look...

Python Documentation

cpython doc: implementing descriptors
@ep12 ep12 added the C-compat A discrepancy between RustPython and CPython label Apr 2, 2020
@coolreader18
Copy link
Member

Thanks for the bug report, this definitely looks weird! I'll look into it.

@coolreader18
Copy link
Member

Oh, I think CPython calls __set_name__ on all of the descriptors in a class declaration, that shouldn't be too hard to do.

@coolreader18 coolreader18 mentioned this issue Apr 2, 2020
Merged
@coolreader18
Copy link
Member

Fixed (I think) in #1835

@ep12
Copy link
Author

ep12 commented Apr 2, 2020

I don't know that much about the python spec / implementation in this regard, but my code works with #1835!
(It fails a couple of lines below...
Can I open a series of issues until my code works with rustpython? I know this project is young, but maybe those real world tests would be a good thing?
My small project works better with pypy than with the reference cpython so I hope that one day (soon) rustpython will work as well as pypy for this code 😄.
However, it might be a real series of issues, the next one being AttributeError: 'code' object has no attribute 'co_varnames' which is definitely more work-intensive.
If you think that rustpython is not in a "make it fully compliant" stage, I wouldn't pollute your issues when you first want to get the most-used features of python cpython compliant...)

Apart from that rfc a big thank you for your work! That was quick! Wow!

@coolreader18
Copy link
Member

coolreader18 commented Apr 2, 2020 •
edited
Loading

Can I open a series of issues until my code works with rustpython?

I think that would be fine! As you said, testing a real world project on RustPython would definitely be a good kind of stress-test.

AttributeError: 'code' object has no attribute 'co_varnames'

I actually just implemented that yesterday in #1834 😄. It should be merged soon.

@ep12 ep12 mentioned this issue Apr 3, 2020
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-compat A discrepancy between RustPython and CPython
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Call __set_name__ on descriptors when initializing types RustPython/RustPython
2 participants
@ep12 @coolreader18