-
Notifications
You must be signed in to change notification settings - Fork 170
Initial support for @pythoncall decorator #1863
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
Conversation
ffac7b7
to
b7ad94f
Compare
Example: $ cat examples/expr3.py
def main0():
n: i32 = 5
m: i32 = fun_from_py_module(n)
print("hi")
print("m recieved is: ", m)
print("this time value received is:", fun_from_py_module(-22))
print("And this time value received is:", fun_from_py_module(101))
print("cpython_average(2, 3): ", cpython_average(2, 3))
print("cpython_average(-100, -1): ", cpython_average(-100, -1))
cpython_print_str("hi how are you?")
pqr: str = "abcdefgh"
cpython_print_str("hi how are you?: " + pqr)
abc: str = cpython_return_random_str()
print("abc: ", abc)
print(cpython_str_concat(abc, pqr))
@pythoncall(module="py_module")
def cpython_str_concat(a: str, b: str)->str:
pass
@pythoncall(module="py_module")
def fun_from_py_module(n: i32) -> i32:
pass
@pythoncall(module="py_module")
def cpython_average(n: i32, m: i32) -> f64:
pass
@pythoncall(module="py_module")
def cpython_print_str(x: str):
pass
@pythoncall(module="py_module")
def cpython_return_random_str() -> str:
pass
main0()
$ cat py_module.py
def fun_from_py_module(n):
import os
print(os.getcwd())
print("hi from cpython", n)
return n + 2
def cpython_average(n, m):
return (m + n) / 3
def cpython_print_str(x: str):
print("I received the string:", x)
def cpython_return_random_str() -> str:
return "this is some string"
def cpython_str_concat(a: str, b: str)->str:
return "concatenated: " + a + " " + b
$ lpython examples/expr3.py --backend c
/Users/ubaid/Desktop/OpenSource/lpython
hi from cpython 5
hi
m recieved is: 7
/Users/ubaid/Desktop/OpenSource/lpython
hi from cpython -22
this time value received is: -20
/Users/ubaid/Desktop/OpenSource/lpython
hi from cpython 101
And this time value received is: 103
cpython_average(2, 3): 1.666667
cpython_average(-100, -1): -33.666667
I received the string: hi how are you?
I received the string: hi how are you?: abcdefgh
abc: this is some string
concatenated: this is some string abcdefgh Note: |
b7ad94f
to
44ea629
Compare
Work In Progress (WIP): CI Tests may/might not pass. |
8cf5a79
to
ccf52ab
Compare
Example from #703 (comment), $ cat examples/expr2.py
@pythoncall(module="py_module")
def f(n: i32) -> str:
pass
def main0():
p: str
p = f(3)
print(p)
main0()
$ cat py_module.py
def f(n) -> str:
import sympy
sympy.var("x")
e = ((x+5)**n).expand()
return str(e)
$ lpython examples/expr2.py --backend c
x**3 + 15*x**2 + 75*x + 125 |
src/bin/lpython.cpp
Outdated
std::string py_version = "3.10"; | ||
std::string py_flags = R"(-I $CONDA_PREFIX/include/python)" + py_version + R"( -L$CONDA_PREFIX/lib -Wl,-rpath -Wl,$CONDA_PREFIX/lib -lpython)" + py_version + R"()"; | ||
cmd += " " + py_flags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be optional. We need these flags when there is a @pythoncall()
in the user source code. Shall we add a compiler flag --enable-cpython-runtime
? We also need the python version here so that we can link to the pythonlib
. How do we get the python version that is installed on the system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just --enable-cpython
and the Python version can be either specified at the command line. Alternatively we can specify the include path and the library name and path directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the flag --enable-cpython
. Currently, I hardcoded the python_version and the include paths for python library.
How do we add support for import path ( The examples in #1863 (comment) and #1863 (comment) do not need |
678e3f3
to
fa95afe
Compare
break; | ||
} | ||
case ASR::ttypeType::Character: { | ||
type_src = "(char*)PyUnicode_AsUTF8"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyUnicode_AsUTF8()
returns a const char *
. We are casting it here to char *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should create a copy of the const char*
string. I think/guess we can support it in another PR.
integration_tests/bindc_03.py
Outdated
@@ -7,14 +7,14 @@ | |||
class ArrayWrapped: | |||
array: CPtr | |||
|
|||
@ccall(header="bindc_03b.h") | |||
@ccall(module="bindc_03b.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call it a header
, I think it's more intuitive what it means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @pythoncall(module="py_module")
usage is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I reverted it back to using header
. I added support for both the keywords header
and module
.
This looks great, thanks @Shaikh-Ubaid for working on this. Let's figure out how to test this, probably we need to add some new "label" into the integration_tests/CMakeLists.txt for this. |
Support decorator @ pythoncall()
fa95afe
to
729a709
Compare
Ready. |
729a709
to
146c657
Compare
integration_tests/CMakeLists.txt
Outdated
@@ -456,6 +476,7 @@ RUN(NAME bindc_05 LABELS llvm c | |||
EXTRAFILES bindc_05b.c) | |||
RUN(NAME bindc_06 LABELS llvm c | |||
EXTRAFILES bindc_06b.c) | |||
RUN(NAME bindpy_01 LABELS c ENABLE_CPYTHON EXTRAFILES bindpy_01_module.py) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with cpython
? We should try to implement it, to make sure all the syntax works.
integration_tests/bindpy_01.py
Outdated
@@ -0,0 +1,77 @@ | |||
from lpython import i32, i64, u32, u64, f32, f64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have to import pythoncall
from here, and implement it in cpython mode.
@@ -1246,6 +1246,11 @@ int link_executable(const std::vector<std::string> &infiles, | |||
cmd += " -I " + rtlib_header_dir; | |||
cmd += " -L" + base_path | |||
+ " -Wl,-rpath," + base_path + " -l" + runtime_lib + " -lm"; | |||
if (compiler_options.enable_cpython) { | |||
std::string py_version = "3.10"; | |||
std::string py_flags = R"(-I $CONDA_PREFIX/include/python)" + py_version + R"( -L$CONDA_PREFIX/lib -Wl,-rpath -Wl,$CONDA_PREFIX/lib -lpython)" + py_version + R"()"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine for now. Conda is currently required.
I think this looks good. I would still get cpython working. That way the test will be Regarding how thing are handled at the ASR level, @czgdp1807 can you please review this? I think we discussed that we would insert explicit type conversions in ASR between our integer and BindPython integer. However I think we can also do that in later PRs. |
146c657
to
f47c9c0
Compare
f47c9c0
to
3a62f41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now, good enough to merge.
We can then iteratively improve this.
towards #703.