Interactive shell implementation #2617

Vipul-Cariappa · 2024-03-19T14:23:14Z

Fixes #329

TODOs

Remember global functions and variables declared.
Error message with proper line number and highlighting
Intrinsic functions support
print the value of the evaluated expression
Import other modules
symengine and numpy support
Support complex data types like list, tuple and dict

Got the following things to work:

Vipul-Cariappa · 2024-03-19T14:36:37Z

I want to know how should I proceed with the first to-do point "Remember global functions and variables declared". The problem is as follows

>>> def f():
...   print("called function f")
... 
>>> f()
semantic error: Function 'f' is not declared and not intrinsic
 --> input:1:1
  |
1 | f()
  | ^

Another example:

>>> x: i32 = 10
>>> print(x)
semantic error: Variable 'x' not declared
 --> input:1:1
  |
1 | print(x)
  | ^

The declared function f is lost by the time we evaluate (i.e. parse and execute) the function call to f. We should store its declaration (and x's declaration) somewhere (I believe at the AST level) and include it while parsing the second part/prompt.

I want to know if there are existing methods to do this. Or should I implement it myself. I want something like.
get_all_function_declerations(AST_t*) and get_all_global_declerations(AST_t*), and extend_ast_with_function_decleration(AST_t*, Function_t*) and extend_ast_with_global_decleration(AST_t*, ...)

Shaikh-Ubaid · 2024-03-21T12:33:35Z

I think the idea to solve this is to keep updating the original ASR on every new interactive query run by the user.

On every run, the functions or variables defined in the previous run need to be updated. We remove the body of functions and set their abiType to interactive. Similarly for variables, we mark them as interactive. Later we handle the interactive functions/variables appropriately in the AST->ASR, ASR passes and ASR->backend.

LFortran already supports interactive compilation. You can look into it for reference. Since the ASR passes and backends are shared with LFortran, I think we will mostly need interactive support for AST->ASR in LPython.

Shaikh-Ubaid · 2024-05-08T20:16:45Z

Let's build this iteratively. For this PR, let's get something that works with the interactive shell, add tests for the implemented support and ensure the CI passes. After that, you can send more PRs to improve the existing support and/or add more features as planned in #2617 (comment). Ideally, small and frequent PRs that just focus on one particular support with robust tests are preferable.

For tests, you can look into how lfortran does it https://github.com/lfortran/lfortran/blob/582db3d4379eee26cfeb0721d5fda21971aaae3d/src/lfortran/tests/test_llvm.cpp#L439-L455.

Feel free to reach out if you need any help.

Shaikh-Ubaid · 2024-05-08T20:17:45Z

Please mark this as "Ready for review" when ready.

Vipul-Cariappa · 2024-05-09T04:13:04Z

It makes more sense to have multiple smaller PR than a large one. I will finish up the work with remembering context from the previous cells, i.e. remembering global functions and variables.
We can add in tests and merge this PR. The other points can be their own separate PRs.

Vipul-Cariappa · 2024-05-10T03:38:34Z

src/lpython/python_evaluator.cpp

    Result<std::unique_ptr<LCompilers::LLVMModule>> res
        = asr_to_llvm(asr, diagnostics,
            e->get_context(), al, lpm, compiler_options,
-            run_fn, infile);
+            "", infile); // ??? What about run function


The argument "" is the run_fn. The run_fn is not used here. In LFortran all global statements are compiled into 1 function, but in LPython it is compiled into two different functions one for global initialization and the other to execute global statements.

I see. Do you need the above change of run_fn to "" for the interactivity to run/work? Or is the above just a refactoring change?

If it is a refactoring change (which it seems to be), I suggest we do it in a separate PR.

Now that we have merged global_init and global_stmts, we can assign run_fn to global_stmts and use it.

Then that change would be part of this PR. You can have a look at it in successive commits.

Vipul-Cariappa · 2024-05-10T03:43:07Z

src/lpython/semantics/python_ast_to_asr.cpp

+        static size_t interactive_execution_count = 0; // ???: should this be a class member variable
+        // interactive_execution_count should be the same as eval_count in PythonCompiler


Any suggestions on this?
Should we create run_fn1 and run_fn2 and share it through the PassOptions?
But even that may not be straightforward.

Another option would be to combine the init and stmts functions. Look at #2573

I saw the linked issue. If combining init and stmt function solves your issue, then you can try it. Submit it as a separate PR and we can rebase this after that is merged.

Vipul-Cariappa · 2024-05-10T03:45:51Z

I guess we can merge the work done till this point. The redefinition of symbols is not yet supported. It looks more complicated than initially thought. We can add support for the redefinition of symbols in a later PR.

Not sure why the tests are failing.

I have left 2 comments please have a look at it.

Shaikh-Ubaid · 2024-05-10T04:57:22Z

You need to update the reference tests. You can do ./run_tests.py -u.

Shaikh-Ubaid · 2024-05-10T05:05:49Z

At a high level, it looks good. I will review in detail in evening today.

Shaikh-Ubaid

I shared a couple comments.

Mainly:

Let's reuse Module, that way we get to reuse a lot of other existing code
We can merge global_init and global_stmts in a separate PR. I think we need it to solve [Bug] All global variable assignments should not go to __main__global_init #2573
Minor refactoring

Apart from that, it looks good. Great start! Amazing work!

src/bin/lpython.cpp

Shaikh-Ubaid · 2024-05-11T01:58:47Z

src/lpython/python_evaluator.cpp

    Result<std::unique_ptr<LCompilers::LLVMModule>> res
        = asr_to_llvm(asr, diagnostics,
            e->get_context(), al, lpm, compiler_options,
-            run_fn, infile);
+            "", infile); // ??? What about run function


I see. Do you need the above change of run_fn to "" for the interactivity to run/work? Or is the above just a refactoring change?

If it is a refactoring change (which it seems to be), I suggest we do it in a separate PR.

src/lpython/parser/parser.cpp

Shaikh-Ubaid · 2024-05-11T02:22:40Z

src/lpython/semantics/python_ast_to_asr.h

@@ -8,7 +8,7 @@ namespace LCompilers::LPython {

    Result<ASR::TranslationUnit_t*> python_ast_to_asr(Allocator &al, LocationManager &lm, SymbolTable* symtab,
        LPython::AST::ast_t &ast, diag::Diagnostics &diagnostics, CompilerOptions &compiler_options,
-            bool main_module, std::string module_name, std::string file_path, bool allow_implicit_casting=false);
+            bool main_module, std::string module_name, std::string file_path, bool allow_implicit_casting=false, bool is_interactive=false);


You can also pass the eval_count here and have it as a class member. Then for body visitor inside visit_Module (assuming we are reusing this instead of defining visit_Interactive), you can use the eval_count for naming global_init and global_stmts if interactive = true. (For example simply append the eval_count to the end if interactive is true).

src/lpython/semantics/python_ast_to_asr.cpp

src/lpython/python_evaluator.cpp

src/bin/lpython.cpp

Shaikh-Ubaid · 2024-05-11T05:38:05Z

Please mark as "Ready for review" when ready.

Shaikh-Ubaid

Good! Few more minor comments. I think it will be good to merge after that. We can add tests in a separate PR.

Shaikh-Ubaid · 2024-05-11T17:54:03Z

src/lpython/python_evaluator.cpp

+#include "libasr/asr_scopes.h"
+#include "lpython/python_ast.h"


Let's use <> instead of "" here and place this along with other similar includes below.

Shaikh-Ubaid · 2024-05-11T17:57:36Z

src/bin/lpython.cpp

@@ -1778,6 +1935,7 @@ int main(int argc, char *argv[])
        compiler_options.use_colors = !arg_no_color;
        compiler_options.indent = !arg_no_indent;

+


Suggested change

Shaikh-Ubaid · 2024-05-11T18:01:01Z

src/lpython/python_evaluator.cpp

 #include <libasr/codegen/asr_to_cpp.h>
 #include <libasr/exception.h>
 #include <libasr/asr.h>
+#include <lpython/pickle.h>
+#include <lpython/parser/parser.h>
+#include <string>


I think this should be included with other system headers at the top. I am unsure if system headers should be listed first and then the custom headers or the other way round. But let's atleast place them together.

Shaikh-Ubaid · 2024-05-11T18:10:08Z

src/lpython/python_evaluator.cpp

-    eval_count++;
-    run_fn = "__lfortran_evaluate_" + std::to_string(eval_count);
-


We later plan to move the interactivity support into the LibASR so that it is shared between LFortran, LPython and LC. So, it is better that we have/use as common/generic code as possible which easily gets shared across all LCompilers.

Assuming it does not affect the newly added interactivity, I think for now it is better to have it.

Can we keep this as part of the next PR? It may require changes in more than one place.

I was working towards "print the value of the evaluated expression" (#2617 (comment)) goal, and I think there might be some obstacles to doing this. But is doable.

Can we keep this as part of the next PR?

We can do that.

Shaikh-Ubaid · 2024-05-11T18:11:25Z

src/lpython/python_evaluator.h

@@ -59,10 +71,10 @@ class PythonCompiler
    Allocator al;
 #ifdef HAVE_LFORTRAN_LLVM
    std::unique_ptr<LLVMEvaluator> e;
-    int eval_count;
+    size_t eval_count;


This is passed as argument to python_ast_to_asr(). The call to python_ast_to_asr() is also used by other backends. So, this should be declared outside of #ifdef HAVE_LFORTRAN_LLVM.

PS: The above will fix the CI failure.

Vipul-Cariappa · 2024-05-12T04:15:27Z

I have made the suggested changes. Please have a look. I have also left a comment on one of your suggestions.

Shaikh-Ubaid

It looks good! Thanks for this! Great work!

Shaikh-Ubaid · 2024-05-12T05:28:11Z

src/lpython/python_evaluator.cpp

-    eval_count++;
-    run_fn = "__lfortran_evaluate_" + std::to_string(eval_count);
-


Can we keep this as part of the next PR?

We can do that.

…SymbolTable.visit_Module

Shaikh-Ubaid · 2024-05-12T05:33:14Z

Let's add some tests for this in the next PR!

Vipul-Cariappa force-pushed the interactive branch 3 times, most recently from 5b9a184 to a18035d Compare May 8, 2024 13:45

Vipul-Cariappa marked this pull request as ready for review May 10, 2024 03:34

Vipul-Cariappa commented May 10, 2024

View reviewed changes

Shaikh-Ubaid reviewed May 11, 2024

View reviewed changes

src/bin/lpython.cpp Outdated Show resolved Hide resolved

Shaikh-Ubaid marked this pull request as draft May 11, 2024 05:37

Vipul-Cariappa force-pushed the interactive branch from a36d692 to b61eccf Compare May 11, 2024 11:00

Shaikh-Ubaid reviewed May 11, 2024

View reviewed changes

Vipul-Cariappa marked this pull request as ready for review May 12, 2024 04:13

Shaikh-Ubaid approved these changes May 12, 2024

View reviewed changes

Vipul-Cariappa added 7 commits May 12, 2024 11:00

Initial interactive shell implementation

2653866

remember global functions and variables declared

2360d8e

avoid printing AST, ASR and LLVM IR

c059767

fixed bug where error from previous cell is printed

54685d3

detect decorators as incomplete input

d2f942c

clean up

a54198a

update according to code review suggestion

da5feda

Vipul-Cariappa added 2 commits May 12, 2024 11:00

removed Interactive_t related stuff by merging required changes into …

a33fa28

…SymbolTable.visit_Module

update according to code review suggestion

1505523

Shaikh-Ubaid force-pushed the interactive branch from 8b12ae4 to 1505523 Compare May 12, 2024 05:30

Shaikh-Ubaid enabled auto-merge (squash) May 12, 2024 05:31

Shaikh-Ubaid merged commit 476d9bb into lcompilers:main May 12, 2024
14 checks passed

Vipul-Cariappa deleted the interactive branch May 12, 2024 13:11

Vipul-Cariappa mentioned this pull request May 13, 2024

Interactive Shell: Printing top level expression #2698

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactive shell implementation #2617

Interactive shell implementation #2617

Vipul-Cariappa commented Mar 19, 2024 •

edited

Loading

Vipul-Cariappa commented Mar 19, 2024

Shaikh-Ubaid commented Mar 21, 2024

Shaikh-Ubaid commented May 8, 2024 •

edited

Loading

Shaikh-Ubaid commented May 8, 2024

Vipul-Cariappa commented May 9, 2024

Vipul-Cariappa May 10, 2024

Shaikh-Ubaid May 11, 2024

Vipul-Cariappa May 11, 2024

Vipul-Cariappa May 10, 2024

Vipul-Cariappa May 10, 2024

Shaikh-Ubaid May 10, 2024

Vipul-Cariappa commented May 10, 2024

Shaikh-Ubaid commented May 10, 2024

Shaikh-Ubaid commented May 10, 2024

Shaikh-Ubaid left a comment

Shaikh-Ubaid May 11, 2024

Shaikh-Ubaid May 11, 2024

Shaikh-Ubaid commented May 11, 2024

Shaikh-Ubaid left a comment

Shaikh-Ubaid May 11, 2024

Shaikh-Ubaid May 11, 2024

Shaikh-Ubaid May 11, 2024 •

edited

Loading

Shaikh-Ubaid May 11, 2024

Vipul-Cariappa May 12, 2024

Shaikh-Ubaid May 12, 2024

Shaikh-Ubaid May 11, 2024 •

edited

Loading

Vipul-Cariappa commented May 12, 2024

Shaikh-Ubaid left a comment

Shaikh-Ubaid May 12, 2024

Shaikh-Ubaid commented May 12, 2024 •

edited

Loading

		static size_t interactive_execution_count = 0; // ???: should this be a class member variable
		// interactive_execution_count should be the same as eval_count in PythonCompiler

		#include "libasr/asr_scopes.h"
		#include "lpython/python_ast.h"

		@@ -1778,6 +1935,7 @@ int main(int argc, char *argv[])
		compiler_options.use_colors = !arg_no_color;
		compiler_options.indent = !arg_no_indent;

		eval_count++;
		run_fn = "__lfortran_evaluate_" + std::to_string(eval_count);

Interactive shell implementation #2617

Interactive shell implementation #2617

Conversation

Vipul-Cariappa commented Mar 19, 2024 • edited Loading

TODOs

Vipul-Cariappa commented Mar 19, 2024

Shaikh-Ubaid commented Mar 21, 2024

Shaikh-Ubaid commented May 8, 2024 • edited Loading

Shaikh-Ubaid commented May 8, 2024

Vipul-Cariappa commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vipul-Cariappa commented May 10, 2024

Shaikh-Ubaid commented May 10, 2024

Shaikh-Ubaid commented May 10, 2024

Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid commented May 11, 2024

Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid May 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid May 11, 2024 • edited Loading

Choose a reason for hiding this comment

Vipul-Cariappa commented May 12, 2024

Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaikh-Ubaid commented May 12, 2024 • edited Loading

Vipul-Cariappa commented Mar 19, 2024 •

edited

Loading

Shaikh-Ubaid commented May 8, 2024 •

edited

Loading

Shaikh-Ubaid May 11, 2024 •

edited

Loading

Shaikh-Ubaid May 11, 2024 •

edited

Loading

Shaikh-Ubaid commented May 12, 2024 •

edited

Loading