Access non-self arguments when widget annotations are computed #9410

JaroslavTulach · 2024-03-14T08:21:35Z

Pull Request Description

Fixes #5629 by adding access to arguments with UUIDs when computing widget annotations.

Implementation Nodes

There are three parts to change:

the IDE has to be changed to provide some information about arguments when requesting widget
such information then allows the on annotation to access cache based on names of function arguments
everything then gets glued together in ExecutionService

Checklist

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

All code follows the
Scala,
Java,
style guides.
All code has been tested:
- Unit tests that send the cache argument in and check the behavior of make_join_condition_selector: 6942ef5
- Unit test the IDE behavior

… queried for cache values

app/gui2/src/components/GraphEditor/widgets/WidgetFunction.vue

...ne/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java

...e/runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionService.java

...ime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/InstrumentorBuiltin.java

JaroslavTulach · 2024-03-14T09:51:12Z

Rejected: Send argument expressions, not values

Once @Frizi suggested to

(Good) idea of the map, though what we would send is probably the argument expressions, not values.
Would it be possible to use the lazy evaluation here?

Turns out that lazy evaluation of arguments isn't possible in this case. To evaluate arbitrary text in place one needs to run the whole program first and only when it reaches the requested point (and all local variables are available in stack frames) one can evaluate the expression.

Looks like this is not how widgets get evaluated, right @4e6? The evaluation is executed outside of any program stack and only obtains self value (identified by UUID) from a cache. This PR proposes to make also other variables available, but they still need to be identified by UUID and be available in the cache, as there is no local stack to extract the values from.

We could probably evaluate the widgets and visualizations in place while executing the whole program. However that would require bigger changes and possibly contradicts

Evaluate visualization sooner than the whole program finishes #9278

@hubertp is currently working on.

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Widget_Helpers.enso

…oin5629

JaroslavTulach · 2024-04-22T11:45:49Z

There is 15 failures in:

Fixed by 9c31cd4

…oin5629

JaroslavTulach · 2024-04-22T15:30:19Z

distribution/lib/Standard/Visualization/0.0.0-dev/src/Widgets.enso

-            _ -> annotation
+        return_target err = err.payload.target
+        Panic.catch Not_Invokable handler=return_target
+            annotation value cache=cache


The cache argument is "optional". If the function doesn't accept it, Not_Invokable panic is raised. It is caught and the value that refused to accept cache argument is returned instead. As a result it is not necessary to accept the cache argument in dynamic dropdown functions.

that is very subtle!

Subtle, but tested!

jdunkerley

Great to have this. Can't wait to try building it out.

jdunkerley · 2024-04-22T15:32:09Z

distribution/lib/Standard/Visualization/0.0.0-dev/src/Widgets.enso

-            _ -> annotation
+        return_target err = err.payload.target
+        Panic.catch Not_Invokable handler=return_target
+            annotation value cache=cache


that is very subtle!

4e6 · 2024-04-23T08:40:16Z

...ne/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java

-  private final Map<UUID, SoftReference<Object>> cache = new HashMap<>();
+  private final Map<UUID, Reference<Object>> cache = new HashMap<>();
+  private final Map<Object, UUID> valuesToKeys = new WeakHashMap<>();
+  private final Map<UUID, Reference<Object>> expressions = new HashMap<>();


I'm concerned that these new caches are not invalidated.

The invalidation is happening in the compile job

enso/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala

Lines 372 to 383 in d665f4d

Seq(

CacheInvalidation(

CacheInvalidation.StackSelector.All,

invalidateExpressionsCommand,

Set(CacheInvalidation.IndexSelector.Weights)

),

CacheInvalidation(

CacheInvalidation.StackSelector.All,

invalidateStaleCommand,

Set(CacheInvalidation.IndexSelector.All)

)

)

Yes, these two new caches aren't invalidated. However that shouldn't matter much - both are using WeakReference and as such they don't hold the referenced objects in memory more than cache which uses SoftReference - e.g. it is necessary to clean the cache only, the valuesToKeys and expressions are going to be cleaned occasionally then by GC.

Btw. valuesToKeys is a WeakHashMap - e.g. the keys are hold by WeakReference automatically. expressions is using WeakReference explicitly to hold the values.

I can write some tests, if necessary to prove the system is behaving correctly. Other than that there shall be no need to change the code.

Test written in 63c0b20- we don't have an assertGC test in the code base yet, we'll see how flaky it'll be. However the assertGC concept has been sufficiently used and tested already.

...ne/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java

Frizi

It's good enough for a merge, but ideally I'd like the collectArgumentNamesAndUuids logic to be cleaned up a little. GUI team can do that after merging this PR though.

Frizi · 2024-04-23T10:57:13Z

app/gui2/src/util/callTree.ts

+        if (a instanceof ArgumentPlaceholder) {
+          // pass thru
+        } else {
+          process(a.ast, a.argInfo?.name)


Instead of calculating index yourself, it should be passed here as a.index. No need to pass MethodCallInfo to this function.

Alas a.index isn't defined in the callTree.test.ts as far as I can say. Maybe it is defined during runtime, but it is missing in the mocked environment. In any case, I can't change the code unless we want to remove the test. Probably better to have worse code with a test than better code without a test...

Frizi · 2024-04-23T10:58:25Z

app/gui2/src/util/callTree.ts

+    let index = 'self' === mci?.suggestion.arguments[0]?.name ? 1 : 0
+    for (const e of arr) {
+      const notApplied = mci?.methodCall.notAppliedArguments ?? []
+      while (notApplied.indexOf(index) != -1) {
+        index++
+      }
+      if (e.uuid) {
+        m['' + index] = e.uuid
+      }
+      const n: string | undefined = mci?.suggestion.arguments[index]?.name
+      if (n && e.uuid) {
+        m[n] = e.uuid
+      }
+      index++
+    }
+    for (const e of arr) {
+      if (e.name && e.uuid) {
+        m[e.name] = e.uuid
+      }
+    }
+    return m


Again, those separate loops are unnecessary. This whole logic can easily be moved into the process function, directly writing to the m output object. We have uuid, argument name and index all available there.

farmaazon

Added some comments about names. @Frizi is doing main review.

app/gui2/src/components/GraphEditor/widgets/WidgetFunction.vue

app/gui2/src/util/callTree.ts

farmaazon · 2024-04-23T11:00:47Z

app/gui2/src/util/callTree.ts

+        }
+      }
+    } else {
+      // ignore: process(args)


What does it mean? Why do we ignore? Should the process(args) be called normally?

This else looks like it bears some important information, but I cannot decipher it here.

Neither I. I don't know how to write a test for this case. What code one has to parse for FromInterpreterWithInfo to not be ArgumentApplication?

AFAIK for example the call foo.bar, where bar takes only self argument.

app/gui2/src/util/callTree.ts

JaroslavTulach · 2024-04-24T01:58:22Z

Let's move on, give libs guys access to Table.join right argument and fix problems as they appear. Thank you for your patience.

JaroslavTulach added 3 commits March 14, 2024 09:12

Collect argument UUIDs when invoking visualization

156b2c1

Add make_join_condition_selector a cache=Nothing argument that can be…

a77f49c

… queried for cache values

Infrastructure to pass cache values to visualizations

171beca

JaroslavTulach self-assigned this Mar 14, 2024

JaroslavTulach requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, 4e6, hubertp, Akirathan, Frizi, farmaazon, vitvakatu and kazcw as code owners March 14, 2024 08:21

JaroslavTulach commented Mar 14, 2024

View reviewed changes

app/gui2/src/components/GraphEditor/widgets/WidgetFunction.vue Outdated Show resolved Hide resolved

JaroslavTulach commented Mar 14, 2024

View reviewed changes

...ne/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java Outdated Show resolved Hide resolved

JaroslavTulach commented Mar 14, 2024

View reviewed changes

...e/runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionService.java Outdated Show resolved Hide resolved

JaroslavTulach commented Mar 14, 2024

View reviewed changes

...ime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/InstrumentorBuiltin.java Outdated Show resolved Hide resolved

radeusgd reviewed Mar 14, 2024

View reviewed changes

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Widget_Helpers.enso Outdated Show resolved Hide resolved

radeusgd reviewed Mar 14, 2024

View reviewed changes

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Widget_Helpers.enso Outdated Show resolved Hide resolved

enso-bot bot mentioned this pull request Mar 15, 2024

Generate completion of Table.join join criteria using data from both joined tables #5629

Closed

JaroslavTulach added 7 commits April 8, 2024 14:55

Merge remote-tracking branch 'origin/develop' into wip/jtulach/TableJ…

21dca9e

…oin5629

Merge remote-tracking branch 'origin/develop' into wip/jtulach/TableJ…

c65f51c

…oin5629

Merge remote-tracking branch 'origin/develop' into wip/jtulach/TableJ…

fc00399

…oin5629

Collecting UUIDs via interpreted.value

a6fb11d

Merge remote-tracking branch 'origin/develop' into wip/jtulach/TableJ…

448990c

…oin5629

Process all but last argument

81aa216

Consolidate annotation processing

6786250

JaroslavTulach added 2 commits April 22, 2024 07:20

Properly type State argument

e9cd67f

Storing RuntimeCache in State

d25b9e6

JaroslavTulach requested review from hubertp, radeusgd and Frizi April 22, 2024 09:20

JaroslavTulach changed the title ~~PoC: Access arguments when widgets annotations are computed~~ Access non-self arguments when widget annotations are computed Apr 22, 2024

Note in changelog

19a7d24

JaroslavTulach added 4 commits April 22, 2024 13:51

Merge remote-tracking branch 'origin/develop' into wip/jtulach/TableJ…

6c9beab

…oin5629

Keep all expressions in a separate map

9c31cd4

Rewriting RuntimeCacheTest to Java

6b349e4

Test RuntimeCache.apply behavior

fd686e9

hubertp approved these changes Apr 22, 2024

View reviewed changes

JaroslavTulach commented Apr 22, 2024

View reviewed changes

jdunkerley approved these changes Apr 22, 2024

View reviewed changes

GregoryTravis approved these changes Apr 22, 2024

View reviewed changes

More sane tests and better results

04b2a91

4e6 reviewed Apr 23, 2024

View reviewed changes

JaroslavTulach requested a review from 4e6 April 23, 2024 09:01

Frizi approved these changes Apr 23, 2024

View reviewed changes

farmaazon reviewed Apr 23, 2024

View reviewed changes

JaroslavTulach added 4 commits April 23, 2024 14:37

Verify expressions don't keep cached values in memory

63c0b20

Better names for argsExternalIds

560077d

Better name for n

dea6f5e

Giving variables longer names

ad7f8b3

JaroslavTulach merged commit ff62c1e into develop Apr 24, 2024
36 checks passed

JaroslavTulach deleted the wip/jtulach/TableJoin5629 branch April 24, 2024 01:58

enso-bot bot mentioned this pull request Apr 25, 2024

Report Warning in the CLI Execution #9749

Open

JaroslavTulach mentioned this pull request May 14, 2024

Cannot access "non-node" arguments in drop down widgets evaluation #9941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access non-self arguments when widget annotations are computed #9410

Access non-self arguments when widget annotations are computed #9410

JaroslavTulach commented Mar 14, 2024 •

edited

Loading

JaroslavTulach commented Mar 14, 2024

JaroslavTulach commented Apr 22, 2024 •

edited

Loading

JaroslavTulach Apr 22, 2024 •

edited

Loading

jdunkerley Apr 22, 2024

JaroslavTulach Apr 23, 2024

jdunkerley left a comment

jdunkerley Apr 22, 2024

4e6 Apr 23, 2024

4e6 Apr 23, 2024

JaroslavTulach Apr 23, 2024

JaroslavTulach Apr 23, 2024 •

edited

Loading

Frizi left a comment

Frizi Apr 23, 2024

JaroslavTulach Apr 23, 2024 •

edited

Loading

Frizi Apr 23, 2024

farmaazon left a comment

farmaazon Apr 23, 2024

JaroslavTulach Apr 23, 2024

farmaazon Apr 23, 2024

JaroslavTulach commented Apr 24, 2024

	Seq(
	CacheInvalidation(
	CacheInvalidation.StackSelector.All,
	invalidateExpressionsCommand,
	Set(CacheInvalidation.IndexSelector.Weights)
	),
	CacheInvalidation(
	CacheInvalidation.StackSelector.All,
	invalidateStaleCommand,
	Set(CacheInvalidation.IndexSelector.All)
	)
	)

Access non-self arguments when widget annotations are computed #9410

Access non-self arguments when widget annotations are computed #9410

Conversation

JaroslavTulach commented Mar 14, 2024 • edited Loading

Pull Request Description

Implementation Nodes

Checklist

JaroslavTulach commented Mar 14, 2024

Rejected: Send argument expressions, not values

JaroslavTulach commented Apr 22, 2024 • edited Loading

JaroslavTulach Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley 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

JaroslavTulach Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Frizi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

JaroslavTulach commented Apr 24, 2024

JaroslavTulach commented Mar 14, 2024 •

edited

Loading

JaroslavTulach commented Apr 22, 2024 •

edited

Loading

JaroslavTulach Apr 22, 2024 •

edited

Loading

JaroslavTulach Apr 23, 2024 •

edited

Loading

JaroslavTulach Apr 23, 2024 •

edited

Loading