-
Notifications
You must be signed in to change notification settings - Fork 42
feat: RefreshComponent #141
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
base: main
Are you sure you want to change the base?
feat: RefreshComponent #141
Conversation
|
In Veil, we have a need for widgets where the data is provided incrementally. Do you intend to merge this (or a similar solution) in the near future? |
|
I just opened the demo file from this PR, and did the WidgetGlitch.mp4 |
| catch e => | ||
| if let .internal id _ := e then | ||
| if id == interruptExceptionId then | ||
| return .last <| .text "This component was cancelled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing this in the countToTen demo. To reproduce, uncomment line 33, then check that the counter on line 30 is running, then uncomment line 36. Now line 30 is cancelled. It seems worrying since no shared cancel token is explicitly created here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha interesting! Although it's true that no cancel token is created explicitly, we are implicitly passing around the cancel token that was given to the command in question.
Apparently this cancel token from line 30 gets set when (un)commenting line 36. I find this a bit surprising. I'd have to look more carefully into how/where the cancel tokens usually get passed around.
|
|
||
| end MonadDrop | ||
|
|
||
| instance : Lean.Server.RpcEncodable Unit where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be From/ToJson? We usually don't impl RpcEncodable directly for types that don't do something with RPC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is that I want to be able to have an RPC method that doesn't return anything, called from JavaScript. And the @[server_rpc_method] attribute requires the RpcEncodable instance. Alternatively I'd have to return a String or something else, and then throw it away.
|
|
||
| Warning: the thread that is updating `state` has started running on the Lean server before the | ||
| widget is activated. The advantage is that there is no delay before the computation starts. | ||
| The disadvantage is that if a `RefreshComponent` is reloaded/unloaded too quickly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdym by 'reloaded/unloaded' here? Is the situation just that a tactic that saves this widget (and launches the thread) is elaborated, then we make an edit, and it gets re-elaborated while leaving the previous thread alive? If so, is it not possible to reuse the standard tactic cancellation mechanism? E.g. what happens if you pass the CoreM cancel token instead of one created manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the description to be more clear. The issue is when clicking on expressions in the goal, every shift-click generates a call to mkGoalRefreshComponent. When shift-clicking as fast as possible, there is not enough time for each of the intermediate widgets to load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is when clicking on expressions in the goal, every shift-click generates a call to
mkGoalRefreshComponent.
Ah, if doing that you could also create/cancel cancellation tokens in the outer function (e.g. in cycleSelections). Then you probably wouldn't need the global cancel token map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. what happens if you pass the CoreM cancel token instead of one created manually?
I'm still wondering about this. In particular w.r.t. to "when the infoview gets closed, cancelRefresh isn't run, and I don't know how to fix this." Using the CoreM token wouldn't cause anything to happen when the infoview is closed, of course, but I think we don't care about that. Mainly, we should ensure that no more than one RefreshComponent thread chain is alive at any given time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my solution with storing the cancel token in a IO.Ref ensures exactly that: whenever a new RefreshComponent thread is started, it cancels the previous one. So there can be at most one alive at any given time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This form of cancelling is only used for widgets like cycleSelections, where the widget depends on the selections in the goal. I think that there it makes sense to only load the widget that relates to the most recently selected selection.
For try? we instead would use the cancel token that we get implicitly in elaboration. (But that suffers from the issue that you noted above in the countToTen example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect to be able to interact with multiple goals at once; let's just not use the global state.
Screen.Recording.2025-12-09.at.1.02.35.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you can put an
IO.Refin the props
I tried this, but it looks like I need a ToJson instance. But there is only a RpcEncodable instance for WithRpcRef (IO.Ref IO.CancelToken), not ToJson. This is because I think it should go in the json object in
elab stx:"cycleSelections" : tactic => do
Widget.savePanelWidgetInfo (hash cycleComponent.javascript) (pure <| json% {}) stx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pure <| json% {} is a StateT RpcStore computation, and you should be able to call rpcEncode in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vtec234, now with the cancellation in the way that you suggested for refreshing panel widgets (passing a IO.Ref IO.CancelToken to the widget, such that each reload cancels the previous one), there is still a problem. It is now possible, e.g. in the cycleSelections demo, to unload the widget without the cancel token being triggered at all:
- Interact with the widget to get it to start its loop.
- Press
ctrl + /to comment out the tactic/command.
Now, the loop is still going, but the reference to the corresponding cancel token has been dropped.
I think what should happen is that instead of replacing the implicit cancel token (which we get in elaboration) with this new cancel token, we should stop if either of the cancel tokens was set. That is, we should stop if (1) the user made a new selection in the goal, or (2) if the relevant tactic gets removed or re-elaborated. Currently we just react to (1).
Unfortunately, I don't think it is possible to merge two cancel tokens, and unfortunately, Core.Context stores an Option IO.CancelToken instead of List IO.CancelToken. I think changing this in core would be the easiest fix here.
|
For the flickering that can be seen in the video, when the message disappears, this is actually the In an earlier version of this widget, instead of disappearing for a bit, the message would change into whatever the initial HTML was set to, which gave a more jarring effect. |
|
I've now made some changes so that each panel widget gets to have its own cancellation of computations of previous selections. In the demo I now also discovered a bug in What is remaining is to figure out how to safely have cancellation of widgets like |
|
HI @JovanGerb. Thanks for your work! We have been using your code in Veil to incrementally display verification results, but found that we had to rewrite the code quite significantly, as the current implementation runs out of stack space (stack overflow) in long-running widgets (on the order of 5-15 minutes), due to having recursive calls in non-tail positions. Basically, we rewrote the code to use explicit loops rather than recursion. We also removed We might have lost some of the generality that's necessary for your use-cases with our approach, but I wanted to highlight the issue and a potential solution. Our code is here. |
|
@dranov, thanks for you report! I did indeed not think about stack overflow problems, because in my use case the number of refreshes is fairly small. I've now refactored my code so that it can be used naturally in a |
|
Thanks again for the PR @JovanGerb, and sorry that it's taking a while. I noticed that it still has the issue I mentioned above, namely that if you open |
|
I did implement the fix that you suggested, so if you open I have now added explanations for the two main limitations. I don't really understand what the changes in your new PR do. Will it solve one of these limitations? |
|
By the way, the fact that |
I just tried it again on a fresh clone to be sure, and it still behaves as in here when the same position is opened twice (e.g. the cursor is there, and also the position is pinned). This is certainly fixable by creating a new cancel token every time the widget is constructed in the infoview, but I would rather have builtin mechanisms handle as much of this as possible, not users (hence the other PR).
Sorry about the opacity! I added some more explanation to the description there. But in short, a cancellation token is already included in the RequestContext (and is hence readable in
My proposal includes an auto-cancellation mechanism that cancels requests started by the infoview when it is closed, so it'd fix part of this. There are really two separate issues here:
I haven't looked at the second limitation (earlier elab tokens getting cancelled by later ones) yet.
Yes, I think so, except that it does one extra thing: it turns a synchronous RPC handler into an asynchronous one, i.e., in JS a call to |
|
Aha, I wasn't actually aware of the "pin" feature! I was just testing this by calling separate instances of the widget from Lean, which is now possible. I indeed see no way to, via a Lean change, support the "pin" feature while still cancelling tasks related to old widget instances, so thanks for opening that other PR. It seems that with the request context cancellation, I will need to add checks for cancellation myself. But what I'm worried about is functions like |
This PR defines
RefreshComponent, which allows you to have a refreshing HTML display, which updates whenever the givenRefreshTaskreturns a new HTML.For cancellation, we store an
Option IO.CancelToken. This could be generalized to storing an arbitraryIO Unitfunction, but I don't think that would be very useful.