Skip to content
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

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

Closed
JaroslavTulach opened this issue May 14, 2024 · 9 comments · Fixed by #10065
Closed

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

JaroslavTulach opened this issue May 14, 2024 · 9 comments · Fixed by #10065
Assignees
Labels
-compiler -gui -language-server p-high Should be completed in the next sprint

Comments

@JaroslavTulach
Copy link
Member

There has to be a balance between amount of cached values and optimal memory usage. As such the PR

added following reasoning and provides access only to argument values already assigned to some other node. @jdunkerley points out that's not enough:

make_format_selector table display=Display.Always cache=Nothing =
    columns = cache.if_not_nothing <| cache "columns"

  @columns (Widget_Helpers.make_column_name_multi_selector add_regex=True)
    @locale Locale.default_widget
    @format Widget_Helpers.make_format_selector
    format : Vector (Text | Integer | Regex) | Text | Integer | Regex -> Text | Date_Time_Formatter | Column -> Locale -> Boolean -> Problem_Behavior -> Table ! Date_Time_Format_Parse_Error | Illegal_Argument
    format self columns format:(Text | Date_Time_Formatter | Column)="" locale=Locale.default error_on_missing_columns=True on_problems=Report_Warning =

Workaround:

Are you calling operator.format [ "x", "y" ]?
Try to move [ ... ] into its own node.

@enso-bot
Copy link

enso-bot bot commented May 22, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-05-21):

Progress: - playing with `Widget: #9941

@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to ⚙️ Design in Issues Board May 22, 2024
@JaroslavTulach
Copy link
Member Author

I've created a small project to play with the drop down arguments. Download as AccessArgumentsDropDown.zip and unzip. The code in question is:

from Standard.Base import all
from Standard.Base.Metadata import all
import Standard.Visualization

type T
    @b for_b
    as_list a b = [a, b]

for_b s cache=Nothing =
    me = [ "self", s ]
    a = ["a", cache "a"]
    b = ["b", cache "b"]
    make [1, "dva", me, a, b]

make values:Vector =
    make_option value = Choice.Option value.to_text value.pretty
    dbg values.to_text
    Widget.Single_Choice (values.map make_option) Nothing Display.Always

main =


    integer1 = 11
    node1 = T.as_list 436 1
    [node1, _]

foreign js dbg info = """
    debugger;

The first thing to notice is that evaluation of the @b widget content happens at UUID 83b40fc8-1b02-418e-8e0d-80f153bc2504 (index 496, size 9) which represents T.as_list - however when this node is evaluated, its arguments 436 and 1 aren't yet computed. There is also d872a8e8-47ea-4c0c-b3ef-a20df2ab4c3b which represents the whole T.as_list 436 1. If we want the drop down visualization to access the arguments, shouldn't we evaluate the visualization for the whole expression, @Frizi and @farmaazon?

@JaroslavTulach
Copy link
Member Author

@jdunkerley, @marthasharkey - if you want to experiment with accessing arguments like 436, try it with this patch:

diff --git engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java
index da2f8e2447..985a02d943 100644
--- engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java
+++ engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/RuntimeCache.java
@@ -38,14 +38,8 @@ public final class RuntimeCache implements java.util.function.Function<String, O
       valuesToKeys.put(value, key);
       return true;
     } else {
-      var otherId = valuesToKeys.get(value);
-      var otherValue = cache.get(otherId);
-      if (otherValue != null && otherValue.get() == value) {
-        // if the value is already cached for another UUID, cache it
-        // for this expression UUID as well
-        var ref = new WeakReference<>(value);
-        expressions.put(key, ref);
-      }
+      var ref = new WeakReference<>(value);
+      expressions.put(key, ref);
       return false;
     }
   }

it does seem to work to some extend (values may be outdated, or missing from time to time). It should be good enough for testing.

@farmaazon
Copy link
Contributor

The first thing to notice is that evaluation of the @b widget content happens at UUID 83b40fc8-1b02-418e-8e0d-80f153bc2504 (index 496, size 9) which represents T.as_list

By evaluation you mean on what value get_widget_json is called? Then it's even stranger, because according to our code it should be called on self argument, that is T in this case.

@enso-bot
Copy link

enso-bot bot commented May 23, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-05-22):

Progress: - small Widget repro: https://discord.com/channels/@me/948519891961544714/1242708122607550464

Discord
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented May 23, 2024

The first thing to notice is that evaluation of the @b widget content happens at UUID 83b40fc8-1b02-418e-8e0d-80f153bc2504 (index 496, size 9) which represents T.as_list

By evaluation you mean on what value get_widget_json is called?

Yes. get_widget_json calls into @b annotation.

Then it's even stranger, because according to our code it should be called on self argument, that is T in this case.

You are right the [{"index":{"value":496},"size":{"value":1}},"83b40fc8-1b02-418e-8e0d-80f153bc2504"] node represents T - I misread the location due to strange formatting of the metadata in my text editor.

@enso-bot
Copy link

enso-bot bot commented May 24, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-05-23):

Progress: - ms in log merged : #10016

@JaroslavTulach JaroslavTulach moved this from ⚙️ Design to 🔧 Implementation in Issues Board May 24, 2024
@JaroslavTulach JaroslavTulach moved this from 🔧 Implementation to 👁️ Code review in Issues Board May 24, 2024
@enso-bot
Copy link

enso-bot bot commented May 27, 2024

Jaroslav Tulach reports a new STANDUP for the last Friday (2024-05-24):

Progress: - reactive RuntimeCache PR: #10065

Discord
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board May 27, 2024
@enso-bot
Copy link

enso-bot bot commented May 28, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-05-27):

Progress: - Reactive Cache is in: #10065

Discord
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.
GitHub
Pull Request Description Refactored mutable parts of ModuleScope into builder to make it easier to reduce unnecessary locks. Important Notes

Checklist
Please ensure that the following checklist ha...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler -gui -language-server p-high Should be completed in the next sprint
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants