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

Display cell run history #2898

Open
dmadisetti opened this issue Nov 19, 2024 · 11 comments · May be fixed by #3168
Open

Display cell run history #2898

dmadisetti opened this issue Nov 19, 2024 · 11 comments · May be fixed by #3168
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@dmadisetti
Copy link
Collaborator

Description

I'm working with a large dataset and occasionally running into OOM errors. When the process is killed, there no way on the frontend to determine the last run cell (or cell that was running before OOM)

Suggested solution

Image

  • Clicking the broken link scrolls to the last cell
  • Some styling on the broken cell (just the red error might be fine)

Alternative

No response

Additional context

Happy to put this in myself, just leaving a bookmark. Small QOL improvement

@dmadisetti dmadisetti added enhancement New feature or request good first issue (typescript) Good first issue on the frontend/typescript labels Nov 19, 2024
@mscolnick
Copy link
Contributor

I've wanted to add another helper panel that just lists out a history of all the cell executions and their times and status (success/failure) in a list. That might help solve this as well?

@dmadisetti
Copy link
Collaborator Author

That absolutely works. LMK if there's an existing issue so I can merge it

@dmadisetti dmadisetti changed the title Indicate last run cell on app disconnect Display cell run history Nov 20, 2024
@mscolnick
Copy link
Contributor

I haven't seen one, but this relates: #974.

I am unsure if its a stack/history of runs or a flame graph (or both).

@mscolnick mscolnick removed the good first issue (typescript) Good first issue on the frontend/typescript label Nov 20, 2024
@mscolnick mscolnick added this to the marimo 1.0.0 milestone Dec 6, 2024
@Light2Dark
Copy link
Contributor

Light2Dark commented Dec 9, 2024

hey, happy to take a stab at this. It's kinda complex imo, so understand if you'd like to take it up (& I have no guarantees on my progress haha)

I drew 2 designs but I really like a log-style approach (top one), since it can be quite extensible (imagine expanding each cell run to view attributes in json/yaml format or adding a timeline view like I drew out).

One question I've been struggling with is how to determine the cells that are part of a unique run. Cmiiw, but the frontend currently doesn't know. So maybe the frontend would have some clever logic to aggregate cells that have run, or we could add something like a 'run_id' to the notebook data.

oh, and where the cell run history data should live, but I think ephemeral state is fine for now.

@mscolnick
Copy link
Contributor

mscolnick commented Dec 9, 2024

@Light2Dark i agree the top one is better. i could imagine each run being closed by default (except maybe the top one) and you can expand to see the cell that were run, times, and result (just like you have designed).

for the chart, I am not sure what libraries out there exist/would be good for this, but we do already use vega-lite internally. you could maybe create a chart using the vega spec. here is a sample chart
the keywords to look are likely: gantt chart or timeline chart.

in future, we could add zooming on the chart and panning to filter the list of cells.

@Light2Dark
Copy link
Contributor

Light2Dark commented Dec 11, 2024

Image

The lib is good! Although the chart has to be built separately and then placed next to the trace rows (so alignment is fiddly). Can't really build it row by row like a react component afaik.

@mscolnick
Copy link
Contributor

mscolnick commented Dec 11, 2024

wow that looks great! i wonder if its would be better to stack them. it wouldn't be aligned then, but you would get:

  • you can pan and zoom the full graph to filter the individual cells below it.
  • scales way better, e.g. past 20 cells
  • more horizontal realstate for the chart itself. (we are rich on vertical realestate

You can also add higlight-on-hover synced when hovered over a bar or the cell summary

@Light2Dark
Copy link
Contributor

Light2Dark commented Dec 11, 2024

hmm, do you have any sample / inspo on the stacked view? Is that a timeline view?

another thing, I'm fixing the chart width and the library is smart enough to readjust each bar's width proportionally. We could put the chart underneath the trace rows to save horizontal space (and maybe make this option a config).

higlight-on-hover synced

yeah that's ideal, that will require some deep integration with vega-lite that I'm not sure is possible. But I'll explore it.

@mscolnick
Copy link
Contributor

I was thinking the browser network tab is good inspo, which does both above and to the right. The above allows for pan/zoom. But i find the right-side is easier to parse (as you've done)

Image

So maybe we do both, eventually, but start with one for now.

@Light2Dark
Copy link
Contributor

Light2Dark commented Dec 12, 2024

cool! I think it's similar to what I've done, since they clamp the total width. I'm exploring putting it below the trace rows.

I found a very nice gantt chart that does a lot of what we want, Online Editor

@mscolnick
Copy link
Contributor

thats great! sounds good

I found a very nice gantt chart that does a lot of what we want

Wow i can't believe this was all done with vega. thats pretty impressive. We should definitely use some of it if we can. We may end up hitting a roadblock when wanting to integrate it with some of our React components (tooltips, anchors, icons)

@Light2Dark Light2Dark linked a pull request Dec 14, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants