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

EDA: Remove old vs-code states #986

Closed
SmiteDeluxe opened this issue Apr 4, 2024 · 8 comments · Fixed by #1161
Closed

EDA: Remove old vs-code states #986

SmiteDeluxe opened this issue Apr 4, 2024 · 8 comments · Fixed by #1161
Assignees
Labels
enhancement 💡 New feature or request released Included in a release vscode 🔨 Issues regarding tools like the VS Code extension

Comments

@SmiteDeluxe
Copy link
Contributor

Is your feature request related to a problem?

Currently the global vs-code state fills up over time making certain operations slow.

Desired solution

The goal is to save the Date of last update along with the State in the global state. Then in the findCurrentState method where those states are iterated once on a new panel startup anyway, the AST should be searched for the placeholder and pipeline name of the old states and in case it does not find that combination AND the states last update is more than a day ago, it should remove that state from the global state.

Possible alternatives (optional)

Just remove if older than 7 days and do not care about the placeholder existence.

Screenshots (optional)

No response

Additional Context (optional)

No response

@SmiteDeluxe SmiteDeluxe added enhancement 💡 New feature or request vscode 🔨 Issues regarding tools like the VS Code extension labels Apr 4, 2024
@SmiteDeluxe SmiteDeluxe self-assigned this Apr 4, 2024
@SmiteDeluxe
Copy link
Contributor Author

@lars-reimann do you know how to best search for a placeholder in the AST given the pipeline name and placeholder name?

@lars-reimann
Copy link
Member

Do you also know the package that the pipeline is contained in?

@SmiteDeluxe
Copy link
Contributor Author

Not particularly, I mean I could search for it I reckon in some way in the AST but it is not info I have at the moment. @lars-reimann

@lars-reimann
Copy link
Member

OK, the most robust way requires three pieces of information:

  1. The URI of the document that contains the pipeline.
  2. The name of the pipeline.
  3. The name of the placeholder.

Then

  1. Get the document by URI: const document = services.shared.workspace.LangiumDocuments.getDocument(uri)
  2. Get the module: const module = <SdsModule>document.parseResult.value
  3. Find the pipeline with the given name: const pipeline module.getModuleMembers().find(it => isSdsPipeline(it) && it.name === ...
  4. Find the placeholder in the pipeline: const placeholder = getPlaceholderByName(pipeline.body, ...)

@lars-reimann
Copy link
Member

@SmiteDeluxe After trying the EDA for some time, I'd suggest removing the interaction with the globalState to persist panels altogether. While data that is loaded from a file may rarely change, this is not true for derived data. This may change after any change to the program.

Example:

  1. Open an EDA panel for filtered:
pipeline myPipeline {
    val titanic = Table.fromCsvFile("C:/Users/Lars/OneDrive/Desktop/test/titanic.csv");
    val filtered = titanic.filterRows((param1) ->
        param1.getValue("age") as Float >= 10
    );
}
  1. Change the program:
pipeline myPipeline {
    val titanic = Table.fromCsvFile("C:/Users/Lars/OneDrive/Desktop/test/titanic.csv");
    val filtered = titanic.filterRows((param1) ->
        param1.getValue("age") as Float >= 60
    );
}
  1. Open the EDA panel for filtered again. Now the data is outdated. Automatically refreshing outdated data without user interaction is exactly what we try to improve over the cells of notebooks.

While the runner is open in the background, it already takes care of caching for us. We should not duplicate this functionality again, trading some reduction in IPC for increased memory use. Using windowed requests, we can also limit the time needed.

If we want to persist state after the runner is closed, this should be handled by the runner, not here.

lars-reimann added a commit that referenced this issue Apr 8, 2024
### Summary of Changes

Disable the global EDA panel cache for now, so tables get refreshed
after code changes.

See also
#986 (comment).
lars-reimann pushed a commit that referenced this issue Apr 10, 2024
## [0.11.0](v0.10.0...v0.11.0) (2024-04-10)

### Features

* add `toFloat` methods for `Int` and `String` ([#1018](#1018)) ([55a2050](55a2050))
* fine-grained control over inlay hints for parameter names ([#1016](#1016)) ([2667caf](2667caf))
* messaging service ([#1004](#1004)) ([dcf4ecf](dcf4ecf))
* semantic highlighting of block lambda results ([#1011](#1011)) ([228733c](228733c))
* start runner in language server ([#1006](#1006)) ([ef4bb6f](ef4bb6f))
* stubs for int conversions ([#1008](#1008)) ([b635f5a](b635f5a))
* support relative paths ([#1019](#1019)) ([3d3f28d](3d3f28d))
* trigger EDA tool via code lenses ([#1010](#1010)) ([eb6e4b6](eb6e4b6))

### Bug Fixes

* consumption of source maps ([#1005](#1005)) ([ea3da87](ea3da87))
* disable global EDA panel cache ([#1014](#1014)) ([f888027](f888027)), closes [/github.com//issues/986#issuecomment-2042731653](https://github.com/Safe-DS//github.com/Safe-DS/DSL/issues/986/issues/issuecomment-2042731653)
* generation of dynamic member function calls with memoization + propagating of impurity information across chained calls ([#1015](#1015)) ([19015c3](19015c3)), closes [#1012](#1012) [#1013](#1013)
* only spawn one runner process and shut it down properly ([#1009](#1009)) ([2c72cee](2c72cee))
* show EDA tool for `TaggedTable` and `TimeSeries` ([#1017](#1017)) ([ffae98a](ffae98a))
@SmiteDeluxe
Copy link
Contributor Author

I see the value in that, yes @lars-reimann. Of course the caching here is not just that what the Runner is doing or should be doing as it is a lot about the action history, but I agree on that it can be a bit counter intuitive. By not having the global state we are taking away some efficiency and usability in one area and adding safety and usability in another aspect. Although the gain in usability might be of more value here as you say, maybe not for new users that have simple pipelines and work slowly but certainly for those that have big pipelines that change a lot.

@lars-reimann
Copy link
Member

Let's talk about that tomorrow.

lars-reimann added a commit that referenced this issue May 11, 2024
Closes #955
Closes #986

### Summary of Changes

Implemented history/action structure for webview to add new actions to
state and have them (if external and info not already present) send
execute requests to runner that are then cancellable or deployed in
correct order. Only fully working for Plots/Tabs at the moment, that are
on deploy added to tabs state and set as currentIndex. All Tabs and
Table retain their state. Runner uses existing methods in RunnerAPI to
get back to relevant state by executing past manipulating actions and
then returns the result of the new action (only if plots right now).

Tabs can be created by selecting columns and right clicking, zooming in
on profiling images or by creating an empty tab with the plus icon in
the sidebar. There in the guided menu users can change the current Tab
to display other info. At that point the tab will go into building state
and show prompts, loading screens and buttons accordingly. Typings are
adapted to abstract as much as possible (mainly around column count for
tabs, none, one and two) and stores are heavily used for reactivity.

---------

Co-authored-by: Lars Reimann <[email protected]>
Co-authored-by: WinPlay02 <[email protected]>
Co-authored-by: megalinter-bot <[email protected]>
lars-reimann pushed a commit that referenced this issue May 16, 2024
## [0.16.0](v0.15.0...v0.16.0) (2024-05-16)

### Features

* eda plot view ([#1161](#1161)) ([a216743](a216743)), closes [#955](#955) [#986](#986)
* integrate version 0.25.0 of the `safe-ds` Python library ([#1174](#1174)) ([f357c38](f357c38))
* prefix keywords with `^` to treat them as identifiers ([#1172](#1172)) ([90bd47c](90bd47c))

### Bug Fixes

* potential stack overflow when computing types of lambda parameters ([#1173](#1173)) ([d89511e](d89511e))
@lars-reimann
Copy link
Member

🎉 This issue has been resolved in version 0.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lars-reimann lars-reimann added the released Included in a release label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 💡 New feature or request released Included in a release vscode 🔨 Issues regarding tools like the VS Code extension
Projects
Status: ✔️ Done
Development

Successfully merging a pull request may close this issue.

2 participants