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

webR: Support streaming output #175

Merged
merged 6 commits into from
Jul 30, 2024
Merged

webR: Support streaming output #175

merged 6 commits into from
Jul 30, 2024

Conversation

WardBrian
Copy link
Collaborator

This uses some examples from the webR documentation to produce something that looks a lot more like the way we handle output in Python. In particular, outputs (both text and plots) will populate while the script is running, rather than at the end.

Because webR provides this via a async API, I had to use their pattern of a infinite (or at least not obviously terminating) async loop, which I find kind of strange. Getting this right required a bit of extra state management, but the end result is pretty nice to use.

@WardBrian WardBrian requested a review from jsoules July 30, 2024 14:02
@magland
Copy link
Collaborator

magland commented Jul 30, 2024

I tried it out on my machine and it's very smooth for the analysis.R example that we've been working with. It's really helpful to show the console progress during package loading.

The usage also looks nice.

Not sure how much you want me to review that new file. I believe it's doing the right thing, but I can take a closer look if you want.

@WardBrian
Copy link
Collaborator Author

Not sure how much you want me to review that new file. I believe it's doing the right thing, but I can take a closer look if you want.

The only part I feel somewhat uncertain of is the async loop, but I think it is working, and it is based on their examples pretty heavily.

Also, do you know if the useMemos I put in the call sites are actually valuable?

@magland
Copy link
Collaborator

magland commented Jul 30, 2024

The only part I feel somewhat uncertain of is the async loop, but I think it is working, and it is based on their examples pretty heavily.

The only thing I would suggest is to have a way to terminate that infinite loop when the useEffect gets closed. That would require passing in something like const closedHandle = {closed: false}, and then setting closedHandle.closed = true when exiting the useEffect. Then checking for closedHandle.closed in the loop.

Also, do you know if the useMemos I put in the call sites are actually valuable?

In this case, useMemo is not needed since you are only using the individual components in the hook dependencies in useWebR. I think you should pass these arguments directly to useWebR() rather than making a separate variable webRArgs.

@magland
Copy link
Collaborator

magland commented Jul 30, 2024

Changes look good to me.

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get it & it looks reasonable to me.

Comment on lines 88 to 89
`webr::shim_install()
webr::canvas()`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These might be a hair more legible if you throw them into a const somewhere with less indentation & give them a name.

Comment on lines 97 to 101
`if (typeof(data) != "list") {
stop("[stan-playground] data must be a list")
}
.SP_DATA <- jsonlite::toJSON(data, pretty = TRUE, auto_unbox = TRUE)
invisible(.SP_DATA)`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These might be a hair more legible if you throw them into a const somewhere with less indentation & give them a name.

@WardBrian
Copy link
Collaborator Author

@jsoules I used the trick we've used elsewhere to put them in actual .R files so we even get nicer syntax highlighting in our editor now

@WardBrian WardBrian merged commit 72fb472 into main Jul 30, 2024
2 checks passed
@WardBrian WardBrian deleted the webR-streaming-output branch July 30, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants