Compile Simple Python package #1185

Thirumalai-Shaktivel · 2022-10-07T13:04:52Z

Related: #992

Thirumalai-Shaktivel · 2022-10-07T13:43:38Z

test_a.py

+import a
+from a import print_a
+
+print(print_a())
+print(a.print_a())
+print(a.print_b())
+print(a.print_c())


The above code works, Can anyone please review and provide feedback?

I think the design in this PR is not correct
Consider the example
test_a.py

import a

ASR

(TranslationUnit (SymbolTable 1 { a: (Module (SymbolTable 3 { print_a: (ExternalSymbol 3 print_a 5 print_a b [] print_a Public ), print_b: (ExternalSymbol 3 print_b 5 print_b b [] print_b Public ), print_c: (ExternalSymbol 3 print_c 9 print_c c [] print_c Public ) }) a [b c] .false. .false. ), b: (Module (SymbolTable 5 { print_a: (Function (SymbolTable 6 { _lpython_return_variable: (Variable 6 _lpython_return_variable ReturnVar () () Default (Character 1 -2 () []) Source Public Required .false. ) }) print_a [] [(= (Var 6 _lpython_return_variable) (StringConstant "A" (Character 1 1 () []) ) () ) (Return)] (Var 6 _lpython_return_variable) Source Public Implementation () .false. .false. .false. .false. .false. [] [] .false. ), print_b: (Function (SymbolTable 7 { _lpython_return_variable: (Variable 7 _lpython_return_variable ReturnVar () () Default (Character 1 -2 () []) Source Public Required .false. ) }) print_b [] [(= (Var 7 _lpython_return_variable) (StringConstant "B" (Character 1 1 () []) ) () ) (Return)] (Var 7 _lpython_return_variable) Source Public Implementation () .false. .false. .false. .false. .false. [] [] .false. ) }) b [] .false. .false. ), c: (Module (SymbolTable 9 { print_c: (Function (SymbolTable 10 { _lpython_return_variable: (Variable 10 _lpython_return_variable ReturnVar () () Default (Character 1 -2 () []) Source Public Required .false. ) }) print_c [] [(= (Var 10 _lpython_return_variable) (StringConstant "C" (Character 1 1 () []) ) () ) (Return)] (Var 10 _lpython_return_variable) Source Public Implementation () .false. .false. .false. .false. .false. [] [] .false. ) }) c [] .false. .false. ), main_program: (Program (SymbolTable 11 { }) main_program [] [] ) }) [] )

Instead of creating ExternalSymbol in the module a, we have to replace it with the actual Function, variables, ...
Also, we should not create the module b and c symtab.
Reason:
1.

from a import b

Here from a will import the module b, so import b throws b already defined (Solution: we have to skip the import b)
2.

import a print(b.print_a())

The above code should throw an error. but it doesn't, as module a import module b also.

What are your thoughts? @certik @czgdp1807

czgdp1807 · 2022-10-09T06:06:57Z

I think we first need to complete #1065. I have assigned @Shaikh-Ubaid to try completing it.

certik · 2022-10-12T16:49:56Z

@czgdp1807 I think the #1065 is needed, but that is the next step, an optimization. I think at first all we need to do when we see any import statement is to see if the module is already loaded in ASR, and if so, use it; otherwise find the file on the disk and load it to ASR and use it. When compiling a program (application) that depends on 100 dependencies, this approach will work.

The #1065 is for the use case when you want to precompile a library ahead of time. I think the ASR (without optimizations) is platform independent, so a given library can be fully precompiled even into a single ASR binary file, and then when you need anything from the library, you just quickly load that file. We definitely want that, but we can simply slowly do this as an optimization.

certik · 2022-10-12T16:54:41Z

This looks great as a start, so I suggest we wrap this PR up (it's not going to be perfect), and then we'll send more PRs to iteratively improve various aspects of this.

TODO before merging:

move the tests into integration_tests
polish everything, I think no new features need to be added, just the code needs to be maintainable enough

czgdp1807 · 2022-10-12T16:54:55Z

I think at first all we need to do when we see any import statement is to see if the module is already loaded in ASR, and if so, use it; otherwise find the file on the disk and load it to ASR and use it

This step is auto-compiling. One extra thing which #1065 is doing is saving the auto-compiled output in a pyc file. Correct me if I am wrong, so you are saying auto-compile a module if needed and include in the ASR in the TranslationUnit's global scope. Please let me know.

certik · 2022-10-12T16:57:39Z

Two features:

The bigger feature we'll have to implement (later!) is nesting modules. We need to design this well at the ASR level and then make it work. We'll work towards it iteratively. It seems that in Python each module is completely standalone, and thus can be represented at the top level (as we already do in ASR), but we need to put the nesting in the name, so a module in the path a/b/c/d/e.py would have a name a.b.c.d.e.
we need to figure out how to represent things like import a and from a.b.c.d import e where (e is a module). It seems that import a creates a variable a in a symbol table. We should either reuse ExternalSymbol (pointing to a module!) or introduce something like a ModuleSymbol.

certik · 2022-10-12T17:09:31Z

@czgdp1807 I think so. I think we just need to get something working first, then improve iteratively. It seems the saving to .pyc files is not necessary for the first implementation, it is a performance improvement (that we obviously need long term), so we can work on the .pyc file compiling separately.

czgdp1807 · 2022-10-12T17:12:02Z

Sounds good to me. Auto-compile can be enabled easily and then including the module in global scope should also be simple. (AFAICT). So this seems doable.

Thirumalai-Shaktivel · 2022-10-18T07:18:15Z

This PR is ready to go in.
Known issues to fix:

The following should not work:

$ lpython -I /home/thirumalai/Open_Source/lpython integration_tests/test_import_01.py 
A
B
C

Instead do:

$ lpython -I /home/thirumalai/Open_Source/lpython/integration_tests integration_tests/test_import_01.py

The following doesn't work:

$ lpython -I /home/thirumalai/Open_Source/lpython/integration_tests test_import_01.py
$

Thirumalai-Shaktivel · 2022-10-18T07:27:52Z

I got questions:

When to use -I? This feature is not available in CPython (Users do https://stackoverflow.com/questions/67631/how-do-i-import-a-module-given-the-full-path to import packages.)
is -I intended to do lpython -I /home/thirumalai/Open_Source/lpython/integration_tests test_import_01.py? If so, how do we test this in comparison with CPython?
P.S. using this PR lpython integration_tests/test_import_01.py works. Here lpython just looks for packages using the parent_dir path (i.e., integration_tests)

certik · 2022-10-19T03:28:14Z

src/libasr/utils.h

@@ -37,6 +37,7 @@ struct CompilerOptions {
    bool implicit_typing = false;
    bool implicit_interface = false;
    std::string target = "";
+    std::string import_path = "";


This will probably later be std::vector<std::string>, but for now that's fine.

certik · 2022-10-19T03:31:47Z

src/lpython/utils.h

+    #if !defined S_ISDIR
+        #define S_ISDIR(m) (((m) & _S_IFDIR) == _S_IFDIR)
+    #endif
+#endif


This needs to be moved to the .cpp file and later we'll move it to a dedicated file, since we want most of our C++ files to be platform agnostic.

Yup, I will move this to utils.cpp file.

certik

I think in general this looks good, I left some comments. We can later simply pass CompilerOptions into python_ast_to_asr, to avoid the long argument list.

@czgdp1807 go ahead and review this as well.

certik

Looks great!

Thirumalai-Shaktivel marked this pull request as draft October 7, 2022 13:05

Thirumalai-Shaktivel commented Oct 7, 2022

View reviewed changes

Thirumalai-Shaktivel force-pushed the compile_packages branch from 4a38080 to 343f71d Compare October 17, 2022 15:06

Thirumalai-Shaktivel added 3 commits October 18, 2022 12:24

Add an option -I to get import source path

0ad7de6

Add is_directory and path_exits functions (using stat)

535fcd2

Compile simple python packages and load it into the ASR

6292d4f

Thirumalai-Shaktivel force-pushed the compile_packages branch from 467aa08 to 70b8f1c Compare October 18, 2022 07:07

Add tests and register in CMakeList

b12fab0

Thirumalai-Shaktivel force-pushed the compile_packages branch from 70b8f1c to b12fab0 Compare October 18, 2022 10:30

certik marked this pull request as ready for review October 18, 2022 16:58

certik requested a review from czgdp1807 October 18, 2022 19:34

certik reviewed Oct 19, 2022

View reviewed changes

certik approved these changes Oct 19, 2022

View reviewed changes

Move stat include statement to utils.cpp

2d5f401

Thirumalai-Shaktivel added the ready for review PRs that are ready for review label Oct 19, 2022

certik approved these changes Oct 20, 2022

View reviewed changes

certik merged commit d70be71 into lcompilers:main Oct 20, 2022

Thirumalai-Shaktivel deleted the compile_packages branch October 22, 2022 06:17

Shaikh-Ubaid mentioned this pull request Nov 5, 2022

Pass CompilerOptions as argument to python_ast_to_asr() #1272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile Simple Python package #1185

Compile Simple Python package #1185

Thirumalai-Shaktivel commented Oct 7, 2022

Thirumalai-Shaktivel Oct 7, 2022

Thirumalai-Shaktivel Oct 8, 2022 •

edited

Loading

czgdp1807 commented Oct 9, 2022

certik commented Oct 12, 2022

certik commented Oct 12, 2022

czgdp1807 commented Oct 12, 2022

certik commented Oct 12, 2022 •

edited

Loading

certik commented Oct 12, 2022

czgdp1807 commented Oct 12, 2022

Thirumalai-Shaktivel commented Oct 18, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Oct 18, 2022 •

edited

Loading

certik Oct 19, 2022

certik Oct 19, 2022

Thirumalai-Shaktivel Oct 19, 2022

certik left a comment

certik left a comment

Compile Simple Python package #1185

Compile Simple Python package #1185

Conversation

Thirumalai-Shaktivel commented Oct 7, 2022

Thirumalai-Shaktivel Oct 7, 2022

Choose a reason for hiding this comment

Thirumalai-Shaktivel Oct 8, 2022 • edited Loading

Choose a reason for hiding this comment

czgdp1807 commented Oct 9, 2022

certik commented Oct 12, 2022

certik commented Oct 12, 2022

czgdp1807 commented Oct 12, 2022

certik commented Oct 12, 2022 • edited Loading

certik commented Oct 12, 2022

czgdp1807 commented Oct 12, 2022

Thirumalai-Shaktivel commented Oct 18, 2022 • edited Loading

Thirumalai-Shaktivel commented Oct 18, 2022 • edited Loading

certik Oct 19, 2022

Choose a reason for hiding this comment

certik Oct 19, 2022

Choose a reason for hiding this comment

Thirumalai-Shaktivel Oct 19, 2022

Choose a reason for hiding this comment

certik left a comment

Choose a reason for hiding this comment

certik left a comment

Choose a reason for hiding this comment

Thirumalai-Shaktivel Oct 8, 2022 •

edited

Loading

certik commented Oct 12, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Oct 18, 2022 •

edited

Loading

Thirumalai-Shaktivel commented Oct 18, 2022 •

edited

Loading