prototypes: data.py, data.r, analysis.py, analysis.r #94
Conversation
| const [status, setStatus] = useState<'idle' | 'loading' | 'running' | 'completed' | 'failed'>('idle') | ||
|
|
||
| const loadPyodideInstance = useMemo(() => { | ||
| let pyodide: PyodideInterface | null = null |
There was a problem hiding this comment.
Is there any hope of sharing code between this and the data file? Not just for code organization reasons, but I also noticed that this seems to re-download pyodide if you run data.py and then switch and run analysis.py
There was a problem hiding this comment.
We need to make a decision on whether to use the same pyodide instance for data.py and analysis.py. Since they are going to be depending on a different set of packages (analysis.py will potentially be a lot heavier), I suggest that we use two separate instances even if it means we download twice.
There was a problem hiding this comment.
Having the variables available from the data script (or at the very least, the data variable) has some value, I think
| height: number | ||
| } | ||
|
|
||
| const exampleScript = `import sys |
There was a problem hiding this comment.
Just to note, the stanio package will probably also be helpful here in terms of preparing a useful output object from the draws. You can see what I'm doing in the normal python tinystan here:
https://github.com/WardBrian/tinystan/blob/main/clients/python/tinystan/output.py
There was a problem hiding this comment.
I'm wondering whether we should use a pandas dataframe (long form, all chains in one). We want something that will stand the test of time so that old SP projects won't stop working.
There was a problem hiding this comment.
If you want compatibility with existing packages, your best bet will either be something like the tinystan object (which is basically just a dict with ndarray entries), or for maximum interoperability something like ArViz's inferencedata object
There was a problem hiding this comment.
I think simpler is better. And we should also try to make it as consistent as possible between R and Python.
There was a problem hiding this comment.
R is a slightly different story, but it has a similar sort of lock-in by a package called posterior. If whatever we provide is not at least easily convertible to something posterior can understand, it won't be used
There was a problem hiding this comment.
One approach could be to provide the data to the script in as raw a format as possible. Then have a standard first few of lines that load it into posterior, or arviz, or pandas, etc. Those conversion lines would be part of the analysis.py/analysis.r.
I did look into it, but it's a lot less straightforward than for pyodide (a lot of ugly message passing). I suggest we hold off on that until after we are able to incorporate the analysis.py into the UI. |
Sounds good. We may want to look at how the Quarto plugin for webr does things: https://github.com/coatless/quarto-webr/blob/8795c3d75fdc8348734d6c05f1ba6350fb225f8e/_extensions/webr/qwebr-compute-engine.js |
Here is a very minimal example showing a plot loaded in the same way they do it: https://stackblitz.com/edit/vitejs-vite-6wuedv?file=src%2FApp.tsx |
magland
changed the title
|
@WardBrian I have added support for analysis.r based on your hints. |
|
Nice! Trying it out now |
|
A few notes before this can move to the non-prototype stage:
|
If I follow the link in the warning it goes to
I tried lazy importing which resulted in code splitting. However, the split off chunks were just around 16 kb for pyodide and 60 kb for webr... so not worth it. The bulk of the source code seems to be dynamically loaded at runtime.
I agree it would be better to do pyodide in a web worker. But I don't think this should "block" us right now since I wouldn't expect user scripts to be very computationally intensive. You can correct me on that. |
|
Computationally intensive in terms of number-crunching no, but I certainly think a common use case for |
If you feel this is important, I can take a crack at it for |
|
I think it is at least worth figuring out if it is possible during this prototyping phase. The fact that jupyterlite doesn't freeze its UI means there must be something there, but it definitely seems possible that if we see what the worker solution requires we may decide that the cure is worse than the disease |
Okay, I'll do it for data.py. (I'm almost positive that jupyterlite use a much more complex system to produce graphical outputs than simply a |
|
It seems like there are a few different solutions out there, see pyodide/matplotlib-pyodide#6 (comment) Most of them look like something not too dissimilar to the webR code where you can end up with a list of pictures and separately a list of text outputs from a given program, and then you need to draw them back on the main thread. |
|
@WardBrian I've got data.py running with a worker. Do you imagine a need for worker on analysis.py at this point? |
|
Here's a working (but quite messy) example of pyodide + matplotlib in a worker, in a similar style to how we do webR now: https://gist.github.com/WardBrian/d89e939c43134ea405fd68084e702ddf I think it is possible that analysis scripts could take a long time to run (in particular, big plots take a long time sometimes), and this also lets us add a 'cancel' button which seems valuable. This should also work for any package which is built on top of matplotlib like seaborne, though it will be worth testing for that later. |
|
@WardBrian @jsoules wonder if you could try to track down the build error here. I'm stuck. There seems to be a problem importing loadPyodide from the web worker dataPyWorker.ts This works in vite dev, but fails on yarn build. |
|
Google suggests adding to the vite config, but I'd be at least a little nervous about this causing issues with the other workers we already have. It's also possible to have the worker be a non-module worker and use |
This seems to have worked! You definitely have a knack. |
I don't think I'm that far away from having Tetris Dreams for the MDN web worker documentation at this point |
|
Okay I've implemented analysis.py in a worker... and I've also left in the non-worker case for comparison, and in case we want to fall back to that. |
There was a problem hiding this comment.
Some notes on the worker -- overall a much cleaner implementation than my hacky thing, and I think it's definitely the way to go. Even just playing around with the small example program we have loaded, it's a much smoother experience.
With an eye toward this becoming a non-prototype, two questions:
- Should data.py and analysis.py be using the same Worker code? (I lean toward yes, it seems like a reasonable thing to do)
- Should data.py and analysis.py use the same Worker instance? (I lean toward no for an initial implementation)
|
|
||
| // here's where we can pass in globals | ||
| const globals = pyodide.toPy({ _sp_example_global: 5 }); | ||
| const script = MPLPreample + '\n' + code + '\nprint("-----x", len(SP_IMAGES))'; |
There was a problem hiding this comment.
Are we currently imagining that the worker will be destroyed after every run? The preamble could be run as part of initialization if not
There was a problem hiding this comment.
I don't think it hurts to have it here, and in fact it makes things more robust in my mind.
| succeeded = false; | ||
| } | ||
| catch (e) { | ||
| console.error(e); |
There was a problem hiding this comment.
Should we do something other than just log this? Like post an error message?
There was a problem hiding this comment.
This is now posted to the main thread, and overall stdout, stderr posting has been improved. The display in the output window is also formatted better.
There was a problem hiding this comment.
It does look much better!
The images need to be constrained in size in some way I think -- the arviz example I posted below leads to a very large image which scrolls off the screen
I agree. But I think we should make that merge only when the time is right.
I agree. Different dependencies. And I would guess that the user will not usually use both in a single session. |
|
Here's a good proof-of-concept for analysis.R, straight out of bayesplot's examples: https://mc-stan.org/bayesplot/reference/PPC-scatterplots.html#examples webr::shim_install()
install.packages("bayesplot")
library(bayesplot)
y <- example_y_data()
yrep <- example_yrep_draws()
p1 <- ppc_scatter_avg(y, yrep)
p1
# don't draw line x=y
ppc_scatter_avg(y, yrep, ref_line = FALSE)
p2 <- ppc_scatter(y, yrep[20:23, ], alpha = 0.5, size = 1.5)
p2
# give x and y axes the same limits
lims <- ggplot2::lims(x = c(0, 160), y = c(0, 160))
p1 + lims
p2 + lims
# for ppc_scatter_avg_grouped the default is to allow the facets
# to have different x and y axes
group <- example_group_data()
ppc_scatter_avg_grouped(y, yrep, group)
# let x-axis vary but force y-axis to be the same
ppc_scatter_avg_grouped(y, yrep, group, facet_args = list(scales = "free_x"))This works as is with no modifications to the code. We might want to call |
|
@magland -- How do we proceed from here? I personally feel like the functionality is good enough at this point that it is worth splitting the PR into two that actually integrate into the rest of the site (assuming you didn't intend for this branch to ever be merged directly -- correct me if I'm wrong there). I suspect some more things will jump out to us once it is placed in context, and it is probably a good opportunity to think about code organization etc now that it seems we are in a pretty good place in terms of feature exploration. Thoughts? |
I didn't have a specific plan. My inclination is to just start new PRs and copy the code over, being sure to reference this discussion in the new PRs. In terms of code organization, I don't have strong opinions, and I don't think too much thought needs to be put into it. Just tidy things up. |
That sounds good to me! |
| outputDiv={outputDiv} | ||
| /> | ||
| )} | ||
| <AnalysisPyOutputWindow key={`analysis-py-output-window-${useWorker}`} width={0} height={0} onOutputDiv={setOutputDiv} /> |
There was a problem hiding this comment.
I'm not sure I understand the onOutputDiv structure here. Why the callback vs. just a simple ref from the beginning?
There was a problem hiding this comment.
To me, passing a ref to the div and then utilizing .current on that ref is susceptible to race conditions etc. With the callback, it is much more clear whether the div has been loaded yet, and the state changes on the load. Maybe there's a more standard way to do it, but I have found that this callback mechanism is robust and is more in line with state management that I am familiar with for react.
| } else { | ||
| return pyodide; | ||
| } |
There was a problem hiding this comment.
In the absence of a useState or useRef here, I'm not sure that this could ever be the case?
There was a problem hiding this comment.
You're right. Fixed.
| // worker creation | ||
| useEffect(() => { | ||
| const worker = new Worker(analysisPyWorkerURL, { | ||
| name: "dataPyWorker", |
There was a problem hiding this comment.
This is a bit confusing--is it the data worker or the analysis worker?
| readOnly: boolean; | ||
| width: number; | ||
| height: number; | ||
| outputDiv?: HTMLDivElement | null; |
There was a problem hiding this comment.
Realistically, is it ever going to be okay for this to be null/undefined?
There was a problem hiding this comment.
It's null until the div is loaded and the callback is triggered.
There was a problem hiding this comment.
Sorry, a better answer: I think we want to be able to use this component without specifying an output div.
| const divElement = document.createElement("div"); | ||
| divElement.style.color = "blue"; | ||
| const preElement = document.createElement("pre"); | ||
| divElement.appendChild(preElement); | ||
| preElement.textContent = dd.data; |
There was a problem hiding this comment.
I'd wrap all of this in a function which could be shared with the stderr version below.
Probably preferable to handle the style via a named css class rather than manual styling.
There was a problem hiding this comment.
I suggest this can be cleaned up later.
| // the runPython call is going to be blocking, so we want to give | ||
| // react a chance to update the status in the UI. | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
| const resultToData = (result: any): any => { | ||
| if (result === null || result === undefined) { | ||
| return result; | ||
| } | ||
| if (typeof result !== "object") { | ||
| return result; | ||
| } | ||
| if (result instanceof Map) { | ||
| const ret: { [key: string]: any } = {}; | ||
| for (const k of result.keys()) { | ||
| ret[k] = resultToData(result.get(k)); | ||
| } | ||
| return ret; | ||
| } else if ( | ||
| result instanceof Int16Array || | ||
| result instanceof Int32Array || | ||
| result instanceof Int8Array || | ||
| result instanceof Uint16Array || | ||
| result instanceof Uint32Array || | ||
| result instanceof Uint8Array || | ||
| result instanceof Uint8ClampedArray || | ||
| result instanceof Float32Array || | ||
| result instanceof Float64Array | ||
| ) { | ||
| return Array.from(result); | ||
| } else if (result instanceof Array) { | ||
| return result.map(resultToData); | ||
| } else { | ||
| const ret: { [key: string]: any } = {}; | ||
| for (const k of Object.keys(result)) { | ||
| ret[k] = resultToData(result[k]); | ||
| } | ||
| return ret; | ||
| } | ||
| }; |
There was a problem hiding this comment.
When we're ready to make this production code, we should probably document the expectations of this format & the internal representation (whatever it is).
| import { PyodideInterface, loadPyodide } from "pyodide"; | ||
|
|
||
| let pyodide: PyodideInterface | null = null; | ||
| const loadPyodideInstance = async () => { |
There was a problem hiding this comment.
I think this may have been discussed already, but the code here seems very similar to the analysis worker, and we would likely benefit from combining them (while using separate instances as appropriate).
|
In preparation for incorporating into the UI, this latest commit I did the following. Combined worker code for data.py and analysis.py so there is now one source file, but still two separate worker instances. Created pyodidedWorker.ts (worker thread), pyodideWorkerInterface (main thread), and pyodideWorkerTypes (types for messages, status, etc) Overall I think this implementation is much more solid. |
|
With this last commit I have separated out the console and image outputs for analysis.py, analysis.r and included console output windows for data.py and data.r
|
|
Note: We should probably add a call to https://pyodide.org/en/stable/usage/api/js-api.html#pyodide.loadPackagesFromImports to the pyodide worker to allow users to use other built-in packages that aren't loaded by default. This can actually replace us manually loading numpy, I think, but the special logic is still needed for arviz since that needs to be installed from pip |
|
@WardBrian should I make a draft of the analysis.R from the main branch and then hand it over to you for creating "draws" object? I didn't want to do this simultaneously if you were planning on working on it. |
|
Oh, I wasn’t working on it because I thought you wanted to! I don’t mind either way, if you want to do it that way it sounds good to me |
This is a redo of #74
In addition to being based on the new main branch, the difference is that it uses an unobtrusive method for triggering the prototypes view.
See this comment
So you would do http://localhost:3000?prototypes=1
The idea here is to be able to develop the functionality of generating data.json using data.py or data.r (and also create plots using analysis.py) and then later decide how to integrate it into the UI. It think it would be a mistake to try to do both at the same time.