Skip to content

Support syntax for passing struct in CPython #1814

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 16, 2023

Conversation

ubaidsk
Copy link
Collaborator

@ubaidsk ubaidsk commented May 16, 2023

Related - #1799.

$ cat examples/expr2.py 
# file: main.py
from lpython import i32, dataclass

from numpy import empty

@dataclass
class Foo:
     x: i32
     y: i32

def init(foos: Foo[:]) -> None:
    foos[0] = Foo(1, 2)

def main() -> None:
    foos: Foo[1] = empty(1, dtype=Foo)
    init(foos)
    print("foos[0].x =", foos[0].x)

main()
$ lpython examples/expr2.py 
foos[0].x = 1
$ python examples/expr2.py 
foos[0].x = 1

@czgdp1807
Copy link
Collaborator

I am not sure if it fixes #1799 entirely. Could you please add tests from that issue to actually see if they work with your change?

@czgdp1807 czgdp1807 marked this pull request as draft May 16, 2023 14:51
@ubaidsk
Copy link
Collaborator Author

ubaidsk commented May 16, 2023

@czgdp1807 do you mean to add the example in #1799 (comment) as an integration_test?

@ubaidsk ubaidsk force-pushed the cpython_class_item branch from e74a622 to 1988299 Compare May 16, 2023 15:00
@czgdp1807
Copy link
Collaborator

Yes.

@ubaidsk ubaidsk force-pushed the cpython_class_item branch from 1988299 to da37514 Compare May 16, 2023 15:04
@ubaidsk
Copy link
Collaborator Author

ubaidsk commented May 16, 2023

Could you please add tests from that issue to actually see if they work with your change?

@czgdp1807 added it. Thank you for the guidance.

@ubaidsk ubaidsk marked this pull request as ready for review May 16, 2023 15:09
foos[0] = Foo(5, 21)

def main0() -> None:
foos: Foo[1] = empty(1, dtype=Foo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just support the Array[] syntax for arrays of structs, like this:

Suggested change
foos: Foo[1] = empty(1, dtype=Foo)
foos: Array[Foo, 1] = empty(1, dtype=Foo)

Then it should be possible to implement this cleanly in lpython.py as well as LPython.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as a quick fix it is fine. I think it might break if Foo already defines __class_getitem__ or if it supports the [] like syntax. Right now LPython doesn't support it, so it's fine. But in the long run I think we will have to support the Array[Foo] syntax.

@certik certik merged commit 8315bc5 into lcompilers:main May 16, 2023
@ubaidsk ubaidsk deleted the cpython_class_item branch May 16, 2023 17:42
@dylon
Copy link

dylon commented May 16, 2023

This does not work in CPython, right now, because __class_getitem__ returns None, which is not a valid type hint:

File "/home/dylon/Workspace/copperhead/demo-apps/lab4/emulation/gsi_emulation0.py", line 515, in gdl_context_desc_get
    c_p_pointer(ctx_desc_ptr, gdl_context_desc[:])
  File "/home/dylon/mambaforge/envs/lab4_emulation/lib/python3.10/site-packages/gsi_emulation/ltypes.py", line 488, in c_p_pointer
    targettype_ptr = ctypes.POINTER(convert_type_to_ctype(targettype))
  File "/home/dylon/mambaforge/envs/lab4_emulation/lib/python3.10/site-packages/gsi_emulation/ltypes.py", line 223, in convert_type_to_ctype
    raise NotImplementedError("Type cannot be None")

@ubaidsk
Copy link
Collaborator Author

ubaidsk commented May 16, 2023

This does not work in CPython, right now, because class_getitem returns None, which is not a valid type hint:

Please, could you share an example which fails?

@dylon
Copy link

dylon commented May 16, 2023

Sure, this reminds me of a bug I need to report that I worked around: #1817

With the workaround above, the following fails with the message above:

from lpython import CPtr, c_p_pointer, p_c_pointer, dataclass, empty_c_void_p, pointer, Pointer

from numpy import empty

@dataclass
class Foo:
    pass

def bar(foos_ptr: CPtr) -> None:
    foos: Pointer[Foo[:]] = c_p_pointer(foos_ptr, Foo[:])

def main() -> None:
    foos: Foo[1] = empty(1, dtype=Foo)
    foos_ptr: CPtr = empty_c_void_p()
    p_c_pointer(pointer(foos), foos_ptr)
    bar(foos_ptr)

main()
$ PYTHONPATH=/home/dylon/Workspace/lpython/src/runtime/lpython python /var/tmp/main.py
Traceback (most recent call last):
  File "/var/tmp/main.py", line 18, in <module>
    main()
  File "/var/tmp/main.py", line 16, in main
    bar(foos_ptr)
  File "/var/tmp/main.py", line 10, in bar
    foos: Pointer[Foo[:]] = c_p_pointer(foos_ptr, Foo[:])
  File "/home/dylon/Workspace/lpython/src/runtime/lpython/lpython.py", line 524, in c_p_pointer
    targettype_ptr = convert_type_to_ctype(targettype)
  File "/home/dylon/Workspace/lpython/src/runtime/lpython/lpython.py", line 255, in convert_type_to_ctype
    raise NotImplementedError("Type cannot be None")
NotImplementedError: Type cannot be None

@certik
Copy link
Contributor

certik commented May 16, 2023

It's weird that it works at our CI. I wonder if it is Python version dependent. Or our CI is broken?

@dylon
Copy link

dylon commented May 16, 2023

This almost works, but leads to another bug (again, branching off my workaround from above): #1818

def dataclass(arg):
    def __class_getitem__(idxs):
        return Array(arg, idxs)
    arg.__class_getitem__ = __class_getitem__
    return py_dataclass(arg)

@certik
Copy link
Contributor

certik commented May 17, 2023

I created a dedicated issue for all the bugs reported in the last few comments above: #1819, so that we don't forget about this. We'll investigate and fix it.

@czgdp1807
Copy link
Collaborator

czgdp1807 commented May 17, 2023

The code posted in #1814 (comment) will not work with CPython because for dataclasses because numpy just creates an array of Python objects. See below,

from lpython import dataclass, i32

from numpy import empty

@dataclass
class Foo:
    x: i32
    y: i32

@dataclass
class Foe:
    x: i32
    y: i32
    z: i32

def main() -> None:
    foos = empty(2, dtype=Foo)
    print(foos.dtype)
    foos[0] = Foo(1, 2)
    foos[1] = Foe(3, 4, 5)
    print(foos[0], foos[1])

main()
(lp) 15:11:37:~/lpython_project/lpython % python /Users/czgdp1807/lpython_project/debug.py
object
Foo(x=1, y=2) Foe(x=3, y=4, z=5)

So we won't be able to use ctypes.Structure here because foos[0], foos[1] can easily store objects of different type. Also, foos.dtype is just a Python object. However I can make the following work,

from lpython import CPtr, c_p_pointer, p_c_pointer, dataclass, empty_c_void_p, pointer, Pointer

from numpy import empty

@dataclass
class Foo:
    pass

def bar(foos_ptr: CPtr) -> None:
    foos: Pointer[Foo[:]] = c_p_pointer(foos_ptr, Foo[:])

def main() -> None:
    foos: Foo[1] = empty(1, dtype=Foo)
    foos_ptr: CPtr = empty_c_void_p()
    p_c_pointer(pointer(foos, Foo), foos_ptr)
    bar(foos_ptr)

main()

with the following diff,

diff --git a/src/runtime/lpython/lpython.py b/src/runtime/lpython/lpython.py
index c6c53cf31..a82a190f8 100644
--- a/src/runtime/lpython/lpython.py
+++ b/src/runtime/lpython/lpython.py
@@ -448,6 +448,8 @@ def pointer(x, type_=None):
         type_ = type(x)
     from numpy import ndarray
     if isinstance(x, ndarray):
+        if py_is_dataclass(type_):
+            return x
         return x.ctypes.data_as(ctypes.POINTER(convert_numpy_dtype_to_ctype(x.dtype)))
     else:
         if type_ == i8:

@czgdp1807
Copy link
Collaborator

Also see #1822. Luck by chance I also ended up with #1814 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants