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

Add console text output tab #262

Merged
merged 4 commits into from
Jan 24, 2025
Merged

Add console text output tab #262

merged 4 commits into from
Jan 24, 2025

Conversation

WardBrian
Copy link
Collaborator

Closes #260

I ended up not doing something live because this would dramatically slow down the sampling if a print statement was present in the model block. Now it only slightly slows it down, at the cost of not being live. If a user really wants to monitor the output live, they still can use the developer console.

image

@WardBrian WardBrian requested a review from jsoules January 23, 2025 21:05
@jsoules
Copy link
Collaborator

jsoules commented Jan 23, 2025

Having looked at nothing yet--I assume the massive slowdown for live updates comes from React and rerendering all the time; is it possible we could escape-hatch this by doing some set-inner-html nonsense?

Not saying it'd be a good idea, mind, but...

@WardBrian
Copy link
Collaborator Author

Hm, I thought the slowdown was also on the worker thread, but you may be right. I’ll try tomorrow through the innerHTML side door

@jsoules
Copy link
Collaborator

jsoules commented Jan 23, 2025

No idea if it would help/how much. In the screenshotted example I'm noticing a slowdown of ~2.5 seconds per run over 4-5 runs, for 4 chains, 1k warmup 1k samples, radius 2. (That about doubles the wall clock time, but it's a trivial example.)

Whereas for the SIR model example, with 100 samples, it's a negligible amount--much less than the inter-run variation without the print statement.

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.

This is probably good to go (I tried it out locally as well), but I pointed out a couple opportunities for refinement if you should choose to pursue them.

const { draws, paramNames, computeTimeSec, samplingOpts } = latestRun;
const { samplingOpts, runResult } = latestRun;
if (!runResult || !samplingOpts) return <span />;
const { draws, paramNames, computeTimeSec, consoleText } = runResult;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this line doesn't die when runResult is undefined, then I think it might be clearer to keep the early <span /> return closer to the actual-component return, i.e. move the current l31 to l33.

gui/src/app/SamplerOutputView/SamplerOutputView.tsx Outdated Show resolved Hide resolved
gui/src/app/StanSampler/StanModelWorker.ts Show resolved Hide resolved
@WardBrian WardBrian requested a review from jsoules January 24, 2025 15:25
@WardBrian
Copy link
Collaborator Author

For posterity: innerHTML trickery was faster than reactivity for live streaming of output, but was still a decent factor slower than I think we'd be willing to tolerate

@WardBrian WardBrian merged commit 7232838 into main Jan 24, 2025
2 checks passed
@WardBrian WardBrian deleted the console-output-tab branch January 24, 2025 15:55
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.

Should print statements end up somewhere other than the developer console?
2 participants