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

Record button appears to execute the node twice #10730

Closed
hubertp opened this issue Aug 1, 2024 · 18 comments
Closed

Record button appears to execute the node twice #10730

hubertp opened this issue Aug 1, 2024 · 18 comments
Assignees

Comments

@hubertp
Copy link
Collaborator

hubertp commented Aug 1, 2024

Notice a simple program
Screenshot from 2024-08-01 12-06-45

File test.txt didn't exist before clicking on the record button.
Once enabled this is the contents of the file:

testtest

suggesting that the node was executed twice.

@hubertp
Copy link
Collaborator Author

hubertp commented Aug 1, 2024

The scenario is as follows:

  1. triggering the record button kicks of a full compilation and then execution
  2. GUI sends the attachVisualization request for .write when record button is selected

Re 1: that's all as expected
Re 2: Attaching a visualization also may trigger an execution. So that's also kind of expected.

@hubertp hubertp moved this from ❓New to 🔧 Implementation in Issues Board Aug 1, 2024
@farmaazon
Copy link
Contributor

farmaazon commented Aug 2, 2024

@hubertp does GUI attach visualization even if there is no visualization shown under the node (like in the screenshot)?

@hubertp
Copy link
Collaborator Author

hubertp commented Aug 2, 2024

@hubertp does GUI attach visualization even if there is no visualization shown under the node (like in the screenshot)?

Yes.

@hubertp
Copy link
Collaborator Author

hubertp commented Aug 2, 2024

@4e6 suggested that this could be an opportunity to provide a proper support for record button rather than via this Standard.Base.Runtime.with_enabled_context Standard.Base.Runtime.Context.Output prefix hack.
Therefore rather than triggering it via a text edit (and later attaching a visualization), GUI would send a request to execute a particular node with a specific execution environment. This would also fix #10719

@enso-bot
Copy link

enso-bot bot commented Aug 2, 2024

Hubert Plociniczak reports a new STANDUP for yesterday (2024-08-01):

Progress: More experiments with reducing copying (#10718). Started looking into record button problems instead of visualization. It should be finished by 2024-08-06.

Next Day: Next day I will be working on the #10730 task. More tweaks to passes that do a lot of copying. Debug record button problem.

@enso-bot
Copy link

enso-bot bot commented Aug 4, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-08-02):

Progress: Found the reason for double execution when record button is pressed, discussing with Dmitry on how to address it properly. More tweaks to reduce copying. More profiling of startup. Reviewing 10735. It should be finished by 2024-08-06.

Next Day: Next day I will be working on the #10730 task. Continue the investigation.

@enso-bot
Copy link

enso-bot bot commented Aug 5, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-08-02):

Progress: Fixing CI issues with #10733. Debugging issues when logging in to cloud. Debugging yet another issue with project not loading (ydoc hardcoded timeouts) on windows. Looking into #10730. It should be finished by 2024-08-06.

Next Day: Next day I will be working on the #10730 task. Continue the investigation.

@farmaazon
Copy link
Contributor

@hubertp does GUI attach visualization even if there is no visualization shown under the node (like in the screenshot)?

Yes.

Then I think there is some GUI issue too. We should not attach viz on the entire write node - only on it's this argument, to get widget dynamic configs (maybe we try to get default viz? But I think we should do that only when it's visible anyway).

@enso-bot
Copy link

enso-bot bot commented Aug 6, 2024

Hubert Plociniczak reports a new STANDUP for yesterday (2024-08-05):

Progress: Investigating CI issues related to No_Such_Method transitive failures, reported as #10750. Looking into #10730 only to realize that some parts of it are already implemented. Investigating how to persist environment-per-node information across compilations/project restarts. It should be finished by 2024-08-06.

Next Day: Next day I will be working on the #10730 task. Continue the investigation.

@farmaazon
Copy link
Contributor

Ok, I know what happens on GUI side: when record button is pressed and code changed, we invalidate information about method call. Without method call, we actually don't know how to ask about widgets, so we set our visualization config to null what is then interpreted as request to detach the visualization.

I'm not sure if there is an easy and still sound way to fix that... Maybe we could update visualization. Or just wait for new recording API (without code changes), so there will be no code change and thus no visualization reattachment.

@enso-bot
Copy link

enso-bot bot commented Aug 8, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-08-06):

Progress: Debugging the current implementation of recomputation, no progress on persisting re-execution without text edits. Tinkering with CI to try to reproduce failures. Stumbled upon build failures on a dedicated mac machine, currently blocked. It should be finished by 2024-08-12.

Next Day: Next day I will be working on the #10730 task. Continue the investigation.

@enso-bot
Copy link

enso-bot bot commented Aug 8, 2024

Hubert Plociniczak reports a new STANDUP for yesterday (2024-08-07):

Progress: Change of course re-record button after internal discussions. Current plan: enable caching of self arg (to avoid cleaning caches and recomputation) and figure out a way to persist existing runtime caches. It should be finished by 2024-08-12.

Next Day: Next day I will be working on the #10730 task. Continue the investigation.

hubertp added a commit that referenced this issue Aug 9, 2024
In order for widgets not to invalidate expression's results and trigger
computations, we now cache self argument to which visualizations should
be attached to.

It should help with #10730 but there is still a bug in GUI.
@hubertp hubertp mentioned this issue Aug 9, 2024
4 tasks
@enso-bot
Copy link

enso-bot bot commented Aug 11, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-08-08):

Progress: Enabled caching of self argument, still testing. Continued investigating MacOS failures. Investigating CI failures. Investigating high memory usage for a user's project. It should be finished by 2024-08-12.

Next Day: Next day I will be working on the #10730 task. File a PR with a solution, investigate surprising high-memory usage for tests.

@enso-bot
Copy link

enso-bot bot commented Aug 12, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-08-09):

Progress: PR is ready, investigating memory leaks in runtime-integration tests, #10785 and #10793 respectively. Fighting with CI. It should be finished by 2024-08-12.

Next Day: Next day I will be working on the #10730 task. Finish working on memory leaks, address PR review.

@hubertp hubertp moved this from 🔧 Implementation to 👁️ Code review in Issues Board Aug 13, 2024
mergify bot pushed a commit that referenced this issue Aug 14, 2024
In order for widgets not to invalidate expression's results and trigger computations, we now cache self argument to which visualizations should be attached to.

It should help with #10730 but there is still a bug in GUI.
@hubertp
Copy link
Collaborator Author

hubertp commented Aug 14, 2024

Or just wait for new recording API (without code changes), so there will be no code change and thus no visualization reattachment.

@farmaazon After a length discussion we decided against that.

@farmaazon
Copy link
Contributor

And visualization update also does not fix the problem. But I assumed #10785 fixes the original issue of double-writes?

@enso-bot
Copy link

enso-bot bot commented Aug 14, 2024

Hubert Plociniczak reports a new STANDUP for the provided date (2024-08-12):

Progress: Continued working on memory leaks in tests, cut memory usage by more than half so far. Need to file follow up tickets that would eliminate remaining prolems. Debugging CI problems with nightlies. It should be finished by 2024-08-12.

Next Day: Next day I will be working on the #10751 task. Fix nightlies, work on dockerizing ydoc.

@hubertp hubertp moved this from ⚙️ Design to 📤 Backlog in Issues Board Dec 10, 2024
@JaroslavTulach
Copy link
Member

It seems to work according to @jdunkerley

@github-project-automation github-project-automation bot moved this from 📤 Backlog to 🟢 Accepted in Issues Board Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🟢 Accepted
Development

No branches or pull requests

4 participants