-
Notifications
You must be signed in to change notification settings - Fork 1
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
Compile Model button and CompileContextProvider #191
Conversation
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 looks pretty good to me! A few questions and suggestions
Resolved all conflicts and tested this. |
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.
A few suggestions. Feel like I need to think some more, but I definitely see that having the compilation status as a context simplifies quite a bit of the state-passing.
if (data.stanFileContent !== theStanFileContentThasHasBeenCompiled) { | ||
if (compileStatus === "compiled" || compileStatus === "failed") { | ||
setCompileStatus(""); | ||
setCompiledMainJsUrl(""); | ||
} | ||
} |
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.
Is it worthwhile to do the flip side of this--such that if the stanFileContent
is flipped back to equal the content that has been compiled, we revert to the "compiled" state?
I'm imagining deleting and re-adding a semicolon for instance.
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'd vote against that because (a) it would be challenging to manage that -- you see in this code the compiledMainJsUrl gets cleared (b) it's easy enough to click the compile button again, and it will be a cache hit.
useEffect(() => { | ||
// persist to local storage | ||
localStorage.setItem("stanWasmServerUrl", stanWasmServerUrl); | ||
}, [stanWasmServerUrl]); |
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.
Not sure I understand why we need to persist this to local storage vs. it being part of the context?
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.
When user refreshes the page, or closes the tab and comes back later, we want this to remember the choice.
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 is also the current behavior, the code just moved in this PR)
Good to merge? |
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.
One suggestion but I'm happy to merge
gui/src/app/CompilationServerConnectionControl/ConfigureCompilationServerDialog.tsx
Outdated
Show resolved
Hide resolved
"app/CompilationServerConnectionControl/*", | ||
"app/compileStanProgram/*" | ||
], | ||
"@SpStanc/*": ["app/Stanc/*", "app/CompilationServerConnectionControl/*"], |
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 could debate whether or not CompilationServerConnectionControl should also be grouped with this context, but I'm fine either way in this PR
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 is also in main branch. Let's address separately.
Closes #161
Added a big "Compile Model" button in the sampling area. More intuitive for the user I believe, so they don't need to read a message "stan model not compiled" and then search for the small button above the editor.
In order to do this I needed to lift the compilation state way up... so I created a CompileContext and moved the handleCompile callback hook into CompileContextProvider. This simplified some things - for example, compiledMainJsUrl doesn't need to be passed around in props.
The one awkward thing was needing to pass the validSyntax boolean up to the context from the editor. That's necessary so that the compile button knows when it needs to be disabled. Aside from that, I think everything has become tidier, IMO.
Once the user clicks compile, their mouse is right there for the "Run Sampling" press, which is nice especially when the compilation has been cached.