New Prototype AI Searcher #7146

MichaelMauderer · 2023-06-28T12:10:03Z

Pull Request Description

Adds a new bare-bones AI searcher that can be triggered with cmd+tab. It will interpret the searcher input as a prompt to an AI model and replace the created node with the suggestion that was computed.

Peek.2023-06-28.15-51.mp4

Implements the first step of #7099.

Important Notes

Contains some refactoring that allows us to have multiple controllers side by side. So QA testing should make sure that the Component Browser Searcher is still working as before.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

… searcher.

# Conflicts: # app/gui/src/controller/searcher.rs # app/gui/view/src/project.rs

farmaazon

I think the FRP API of Project View is a bit confusing ATM.

farmaazon · 2023-06-30T07:54:50Z

app/gui/src/controller/graph.rs

+#[allow(missing_docs)]
+#[derive(Clone, Copy, Debug, Fail)]
+#[fail(display = "AST node is missing ID.")]
+pub struct UnsupportedPattern;


The error message is copy-pasted

farmaazon · 2023-06-30T07:56:55Z

app/gui/src/controller/graph.rs

+/// Description of the nodes variable name.
+#[derive(Clone, Debug)]
+pub enum VariableName {
+    /// The node has a variable name.
+    Some(String),
+    /// The node has no variable name.
+    None,
+    /// The node has a variable name, that is not currently supported. For example, it is a
+    /// pattern subpart. See notes on [Node::variable_name].
+    NotSupported,
+}
+


Is it needed? It looks like was meant to be used as variable_name return type, but then replaced with Result<Option<&str>, UnsupportedPattern>.

You are right, this is no longer used. Removed.

farmaazon · 2023-06-30T08:39:06Z

app/gui/src/controller/searcher.rs

-
-    use crate::controller::ide::plain::ProjectOperationsNotSupported;
-    use crate::executor::test_utils::TestWithLocalPoolExecutor;
-    use crate::test::mock::data::project_qualified_name;
-    use crate::test::mock::data::MAIN_FINISH;
-    use crate::test::mock::data::MODULE_NAME;
+    use std::assert_matches::assert_matches;

-    use crate::controller::graph::RequiredImport;
    use engine_protocol::language_server::types::test::value_update_with_type;
    use engine_protocol::language_server::SuggestionId;
    use enso_suggestion_database::entry::Argument;
    use enso_suggestion_database::mock_suggestion_database;
    use enso_suggestion_database::SuggestionDatabase;
    use json_rpc::expect_call;
    use parser::Parser;
-    use std::assert_matches::assert_matches;
+
+    use crate::controller::graph::RequiredImport;
+    use crate::controller::ide::plain::ProjectOperationsNotSupported;
+    use crate::executor::test_utils::TestWithLocalPoolExecutor;
+    use crate::searcher::apply_this_argument;
+    use crate::test::mock::data::project_qualified_name;
+    use crate::test::mock::data::MAIN_FINISH;
+    use crate::test::mock::data::MODULE_NAME;
+
+    use super::*;



These imports look reversed.

farmaazon · 2023-06-30T08:43:52Z

app/gui/src/presenter/project.rs

            self.ide_controller.clone_ref(),
            self.controller.clone_ref(),
            self.graph_controller.clone_ref(),
            &self.graph,
-            self.view.clone_ref(),
+            self.view.clone(),


Why we clone view instead of clone-refing?

Spurious change. Reverted.

farmaazon · 2023-06-30T08:47:34Z

app/gui/src/presenter/searcher.rs

+    // fn view(app: &Application) -> Option<Instance>
+    // where Self: Sized {
+    //     Some(component_browser::View::new(app).display_object().clone_ref())
+    // }


Commented code.

farmaazon · 2023-06-30T08:59:46Z

app/gui/view/graph-editor/src/lib.rs

@@ -3022,7 +3022,6 @@ fn init_remaining_graph_editor_frp(
        );
        out.node_added <+ new_node;
        node_to_edit_after_adding <- new_node.filter_map(|&(id,_,do_edit)| do_edit.as_some(id));
-        eval node_to_edit_after_adding((id) model.with_node(*id, |node| node.show_preview()));


Why is it removed?

Because the AI Searcher does not want to show a preview. So, this has been moved to the init_input_node where we enable this conditionally based on the searcher.

farmaazon · 2023-06-30T09:02:10Z

app/gui/view/src/project.rs

+        // TODO[MM]: this should not contain the group entry id as that is component browser
+        // specific. It should be refactored to be an implementation detail of the component
+        // browser.


Actually, in the new design we plan so far to have just a single view component for both searching entries and asking AI, so there will be only one view.

This comment refers more to the fact that each new searcher (i.e., Component Browser Searcher and the planned combined AI searcher) might have its own separate view with different APIs. Each, which will need to be handled in a generic way on the project level, and interactions between the View and Presenter should be an internal implementation detail.

farmaazon · 2023-06-30T09:03:16Z

app/gui/view/src/project.rs

@@ -472,7 +504,7 @@ impl View {
            eval position ((pos) model.searcher.set_xy(*pos));

            // Showing searcher.
-            searcher.show <+ frp.searcher.is_some().on_true().constant(());
+            searcher.show <+ frp.searcher.unwrap().map(|params| matches!(params.searcher_type, SearcherType::ComponentBrowser)).on_true();


Over 100 characters.

farmaazon · 2023-06-30T09:08:48Z

app/gui/view/src/project.rs

+
+
+            // === Handling Inputs to the AI Searcher and Committing Edit ===
+
+            ai_searcher_active <- frp.searcher_type.map(|t| *t == SearcherType::AiCompletion);
+            committed_in_ai_searcher <- frp.accept_searcher_input.gate(&ai_searcher_active);
+            committed_in_ai_searcher <- committed_in_ai_searcher.map2(&last_searcher, |_, &s| (s.input, None));
+            frp.source.editing_committed <+ committed_in_ai_searcher;


It's strange thataccept_searcher_input FRP input seemingly is used only to accept AI input. The more strange is, that we accept AI input with Tab.

Is the accept_searcher_input relevant?

Unfortunately, it does work without this code. The fact that it uses tab was an oversight, but otherwise this was meant to compensate for the fact that there is no view associated with the AI searcher. But it appears that the Component Browser searcher is receiving the enter event anyway and starting the chain of events that is committing the current open searchers content.

farmaazon · 2023-06-30T09:11:14Z

build-config.yaml

@@ -1,6 +1,6 @@
 # Options intended to be common for all developers.

-wasm-size-limit: 15.97 MiB
+wasm-size-limit: 16.05 MiB


This is a bit odd. Why such a big change from 600-lines of PR?

I don't know, however, what could we do with it. Certainly, it's not like we add a big dependency or make an extensive usage of macros.

I'm not sure. My initial guess would be that some code optimizations are no longer being applied because the searcher is no longer a fixed struct within the project, but a trait. Do we have a good way to debug Wasm size changes?

I'm trying to have a look with twiggy.

farmaazon · 2023-06-30T14:33:17Z

app/gui/src/presenter/searcher/component_browser_searcher.rs

The _searcher suffix in module's name is redundant. Similarly, in ai_searcher.

farmaazon · 2023-06-30T14:36:41Z

app/gui/view/src/project.rs

+        /// Accept the current input of the searcher.
+        accept_searcher_input(),


We still have this input, just not assigned to any shortcut.

And my original comment was mostly about name, as the name did not say about accepting AI searcher only. But if the shortcut from the original searcher works, we can go with it (just remove this input then).

kazcw · 2023-07-03T19:07:27Z

app/gui/src/controller/graph.rs

@@ -127,6 +132,22 @@ impl Node {
    pub fn has_position(&self) -> bool {
        self.metadata.as_ref().map_or(false, |m| m.position.is_some())
    }
+
+    /// Get the nodes variable name, if it has one.


Suggested change

/// Get the nodes variable name, if it has one.

/// Get the node's variable name, if it has one.

kazcw · 2023-07-03T19:14:50Z

app/gui/src/presenter/searcher.rs

-        let target_node = mode.as_ref().map(|mode| mode.node_id());
-        if let Ok(target_node) = target_node {
-            graph.allow_expression_auto_updates(target_node, false);
+        let mode = mode?;


This will always succeed. mode does not need to be a Result.

kazcw · 2023-07-03T19:23:23Z

app/gui/src/presenter/searcher.rs

+        if let Some(target_node_view) = graph_presenter.view_id_of_ast_node(target_node) {
+            // We only want to show the preview of the node if it is a component browser searcher.
+            if matches!(parameters.searcher_type, SearcherType::ComponentBrowser) {


i think this would be clearer with this if around the whole if let.

kazcw · 2023-07-03T19:37:49Z

app/gui/src/presenter/searcher.rs

+/// Create a new AST that combines a `this` argument with the given AS. For example, to add a
+/// method call `sort` a this argument `table`. That would result in an AST that represents


Typos in first two sentences here.

kazcw · 2023-07-03T19:40:12Z

app/gui/src/presenter/searcher.rs

+/// Create a new AST that combines a `this` argument with the given AS. For example, to add a
+/// method call `sort` a this argument `table`. That would result in an AST that represents
+/// `table.sort`.  
+pub fn apply_this_argument(this_var: &str, ast: &Ast) -> Ast {


I don't see that this is searcher-specific; should it go in the ast module?

kazcw · 2023-07-03T19:41:00Z

app/gui/src/presenter/searcher.rs

+    }
+}
+
+/// Initialise the expression in case there is a source node for the new node. This allows us to to


Suggested change

/// Initialise the expression in case there is a source node for the new node. This allows us to to

/// Initialise the expression in case there is a source node for the new node. This allows us to

kazcw · 2023-07-03T19:44:36Z

app/gui/src/presenter/searcher/ai.rs

+            graph_controller,
+            graph_presenter: graph_presenter.clone_ref(),
+            this_arg,
+            input_expression: RefCell::new("".to_string()),


Suggested change

input_expression: RefCell::new("".to_string()),

input_expression: default(),

kazcw · 2023-07-03T19:50:06Z

app/gui/src/presenter/searcher/component_browser.rs

+
+/// The Searcher presenter, synchronizing state between searcher view and searcher controller.
+///
+/// The presenter should be created for one instantiated searcher controller (when node starts to


Suggested change

/// The presenter should be created for one instantiated searcher controller (when node starts to

/// The presenter should be created for one instantiated searcher controller (when node starts

wdanilo

What happens if the user did not set OpenAI key? How the error is reported?

JanBeelte · 2023-07-04T09:47:02Z

What happens if the user did not set OpenAI key? How the error is reported?

currently we get a pretty misleading method_not_found RPC error from the language server. Given we want to get rid of the endpoint completely we did not spend time on proper error handling.

farmaazon · 2023-07-04T12:59:50Z

QA Report 1: 🔴

Playing a little without API key, found two issues:

Having AI prompt, when I press escape, the node remains, with the prompt I wrote so far (may be empty if typed nothing) and Nothing in the code.
Right after opening AI prompt and canceling it, dragging-out edge creates nodes without Component Browser and in a strange state.

Both are blocking if we don't want to use any feature flag for disabling cmd+tab shortcut.

MichaelMauderer · 2023-07-05T15:29:02Z

Both of these points should now be addressed. @farmaazon

farmaazon · 2023-07-06T09:22:00Z

QA Report 2: 🔴

I still have a similar issue to the second point from last report: but this time accepting or cancelling an AI prompt makes future dragged-out edges become AI prompts until CB is opened some other way.

Also, I cannot accept AI prompt in any other way than clicking on the background (tried enter, tab, ctrl+tab - none of those worked).

At the end, I'd like to share you a nice response for my "What question should I ask?"

# Conflicts: # app/gui/view/src/project.rs

MichaelMauderer · 2023-07-06T15:22:31Z

Thanks for the thorough testing. @farmaazon

It appears that the accepting AI prompts with "enter" worked fine only as long as the CB was not opened. Then its view would interfere with the event processing. This is now fixed.

Newly created nodes from dragging will now also use the “default” searcher (CB searcher at the moment).

farmaazon · 2023-07-07T09:32:18Z

QA report 3: 🍏

No more issues found!

MichaelMauderer added 14 commits June 21, 2023 15:47

First prototype of ai searcher implementation.

1245cc3

Update input handling.

3485221

Simplified communication for node editing.

df5038b

Implement correct updates to the project when committing node with AI…

a36c112

… searcher.

Updated shortcuts.

e930865

Prevent preview vis for ai searcher.

6e37b87

Enable preview vis for component browser searcher.

3827557

Correctly show preview and edge for searcher input.

c8595d0

Revert key assignment. Remove logging. Fix lints.

a37a5ee

Merge branch 'develop' into wip/MichaelMauderer/ai_searcher

9aeef9b

# Conflicts: # app/gui/src/controller/searcher.rs # app/gui/view/src/project.rs

Fix merge issues. Clean ThisNode constructor.

8c5f89a

Code cleanup.

92f501b

Code cleanup.

72f5e91

Fmt.

55d18db

MichaelMauderer added the -gui label Jun 28, 2023

MichaelMauderer self-assigned this Jun 28, 2023

Add changelog.

4647364

MichaelMauderer marked this pull request as ready for review June 28, 2023 14:52

MichaelMauderer requested review from wdanilo, farmaazon, kazcw and mwu-tow as code owners June 28, 2023 14:52

MichaelMauderer added 2 commits June 28, 2023 15:59

Fmt.

5771b34

Increase wasm-size-limit.

32fc458

MichaelMauderer marked this pull request as draft June 28, 2023 15:03

MichaelMauderer added 2 commits June 28, 2023 16:41

Fix failing test.

8e45afb

Bump wasm size.

b1c5b5f

MichaelMauderer marked this pull request as ready for review June 28, 2023 22:59

MichaelMauderer mentioned this pull request Jun 28, 2023

Implement AI-powered searcher #7099

Closed

10 tasks

farmaazon requested changes Jun 30, 2023

View reviewed changes

MichaelMauderer added 9 commits June 30, 2023 12:23

Restructure imports.

6022370

Use clone_ref.

e58589c

Remove unused code.

6777480

Restructure file hierarchy for clarity.

eaaab0e

Renaming for clarity.

79832a0

Reduce line length.

1b4cab0

Remove unused functionality.

f2922ed

Bump size limit.

8c2d014

Provide unboxed setup_searcher.

2e593da

MichaelMauderer requested a review from farmaazon June 30, 2023 12:45

farmaazon approved these changes Jun 30, 2023

View reviewed changes

MichaelMauderer added 2 commits July 3, 2023 10:04

Review feedback.

b9c76cc

Remove redundant suffix.

91a44f6

kazcw approved these changes Jul 3, 2023

View reviewed changes

wdanilo approved these changes Jul 3, 2023

View reviewed changes

Review feedback.

b488e15

QA feedback.

b1545dd

MichaelMauderer added 3 commits July 6, 2023 15:31

Revert to component browser after accepting AI input.

a065fbf

Merge branch 'develop' into wip/MichaelMauderer/ai_searcher

dc5dd7a

# Conflicts: # app/gui/view/src/project.rs

Fix accepting of input.

29561f2

MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Jul 7, 2023

mergify bot merged commit 66201dc into develop Jul 7, 2023

mergify bot deleted the wip/MichaelMauderer/ai_searcher branch July 7, 2023 12:47

4e6 mentioned this pull request Nov 27, 2023

Review and update the current state of our AI integration #8348

Closed

		/// Accept the current input of the searcher.
		accept_searcher_input(),

	/// Get the nodes variable name, if it has one.
	/// Get the node's variable name, if it has one.

		/// Create a new AST that combines a `this` argument with the given AS. For example, to add a
		/// method call `sort` a this argument `table`. That would result in an AST that represents

	/// Initialise the expression in case there is a source node for the new node. This allows us to to
	/// Initialise the expression in case there is a source node for the new node. This allows us to

	input_expression: RefCell::new("".to_string()),
	input_expression: default(),

	/// The presenter should be created for one instantiated searcher controller (when node starts to
	/// The presenter should be created for one instantiated searcher controller (when node starts

New Prototype AI Searcher #7146

New Prototype AI Searcher #7146

Conversation

MichaelMauderer commented Jun 28, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

farmaazon left a 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

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

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

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

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

wdanilo left a comment

Choose a reason for hiding this comment

JanBeelte commented Jul 4, 2023

farmaazon commented Jul 4, 2023

QA Report 1: 🔴

MichaelMauderer commented Jul 5, 2023 • edited Loading

farmaazon commented Jul 6, 2023

QA Report 2: 🔴

MichaelMauderer commented Jul 6, 2023

farmaazon commented Jul 7, 2023 • edited Loading

QA report 3: 🍏

MichaelMauderer commented Jun 28, 2023 •

edited

Loading

MichaelMauderer commented Jul 5, 2023 •

edited

Loading

farmaazon commented Jul 7, 2023 •

edited

Loading