Skip to content

Pythoncall: Support arrays of int, real as args #1931

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 5 commits into from
Jun 17, 2023

Conversation

Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid commented Jun 17, 2023

towards #703

This PR supports passing of arrays of types int, real to @ pythoncall decorated functions. Returning arrays of simple types is yet to be supported.

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the pythoncall_arrays_c branch from d3fadd7 to 4f13699 Compare June 17, 2023 09:24
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.5 FATAL_ERROR)
cmake_minimum_required(VERSION 3.15 FATAL_ERROR)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the cmake minimum version here. cmake with version 3.5 is unable to find python from the conda environment (It always picks the system level python). It seems cmake with version 15 and above supports finding python from conda environment (reference https://gitlab.kitware.com/cmake/cmake/-/issues/21322#note_845923). Python from conda environment is needed as the conda environment contains the installed numpy package.

@Shaikh-Ubaid
Copy link
Collaborator Author

Shaikh-Ubaid commented Jun 17, 2023

Doubt related to return types as arrays:

$ cat expr2.py
from lpython import pythoncall, f32

@pythoncall(module = "expr2_module")
def return_multi_dim_arr() -> f32[2, 3]:
    pass

def main0():
    x: f32[2, 3] = return_multi_dim_arr()
    print("x = ", x)

main0()
$ cat expr2_module.py 
import numpy as np

def return_multi_dim_arr():
    a = np.array([3.12, -2.14, 1., 4., 5., 6.]).reshape((2, 3))
    print("CPython: ", a)
    return a

In the above example, the function def return_multi_dim_arr() -> f32[2, 3] gets transformed to def return_multi_dim_arr(struct r32* _lpython_return_variable) (I think via the create_subroutine_from_function() and/or array_op pass). Since it is transformed, its signature does not match to that of its counter-part defined in the expr2_module.py.

How do we tackle this? Shall we skip updating/modifying @pythoncall decorated functions in the create_subroutine_from_function() and array_op pass?

CC: @certik, @czgdp1807, @Smit-create

@Shaikh-Ubaid Shaikh-Ubaid requested a review from certik June 17, 2023 10:04
@@ -1562,6 +1565,7 @@ int main(int argc, char *argv[])
app.add_flag("--get-rtl-dir", print_rtl_dir, "Print the path to the runtime library file");
app.add_flag("--verbose", compiler_options.verbose, "Print debugging statements");
app.add_flag("--enable-cpython", compiler_options.enable_cpython, "Enable CPython runtime");
app.add_flag("--enable-cnumpy", compiler_options.enable_cnumpy, "Enable C-Numpy runtime");
Copy link
Contributor

@certik certik Jun 17, 2023

Choose a reason for hiding this comment

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

I would call it

Suggested change
app.add_flag("--enable-cnumpy", compiler_options.enable_cnumpy, "Enable C-Numpy runtime");
app.add_flag("--enable-numpy", compiler_options.enable_numpy, "Enable NumPy runtime (implies --enable-cpython)");

@certik
Copy link
Contributor

certik commented Jun 17, 2023

Shall we skip updating/modifying @pythoncall decorated functions in the create_subroutine_from_function() and array_op pass?

Yes. I think it skips BindC functions and we just need it to also skip BindPython functions too.

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 this looks great!

@certik certik merged commit 38cd377 into lcompilers:main Jun 17, 2023
@certik
Copy link
Contributor

certik commented Jun 17, 2023

I am going to merge this, as it looks good. In subsequent PRs:

  • Use --link-numpy (see my comment above)
  • Create a dedicated CI and update our integration_tests, that tests everything except python, via C and LLVM backends and ensures we have no dependency on CPython; and add a CI that only tests the Python interoperability tests (for now only C, later LLVM as well)

@Shaikh-Ubaid Shaikh-Ubaid deleted the pythoncall_arrays_c branch June 17, 2023 17:15
@Shaikh-Ubaid
Copy link
Collaborator Author

Use --enable-numpy

We can support this. A minor concern here is that people/users might get confused as numpy is also package that can be imported (In this PR, cnumpy refers to numpy-c library/API). For example, users might think --enable-numpy is used to enable importing numpy, that is, from numpy import empty, .... In actual --enable-numpy is only needed if you are using numpy along with support for cpython runtime.

@certik
Copy link
Contributor

certik commented Jun 17, 2023

I see. Maybe we can call it --link-numpy instead then.

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.

2 participants