materialsproject / crystaltoolkit

Crystal Toolkit is a framework for building web apps for materials science and is currently powering the new Materials Project website.
https://docs.crystaltoolkit.org
Other
149 stars 59 forks source link

`imageRequest` still invalid prop #375

Open janosh opened 11 months ago

janosh commented 11 months ago

@mkhorton I just installed the latest version crystal-toolkit==2023.10.24, hoping https://github.com/materialsproject/mp-react-components/pull/699 will have fixed the write_structure_screenshot_to_file.py script.

https://github.com/materialsproject/crystaltoolkit/blob/cf2c1a9344b0d4811b3facb11d3aaa1a11a902b7/crystal_toolkit/apps/examples/write_structure_screenshot_to_file.py#L10-L18

I just adapted it to run with the new CrystalToolkitPlugin but I still get the same imageRequest invalid prop error

Screenshot 2023-10-24 at 3 17 52 PM

is this not using the latest dash-mp-components?

Programmatic structure screenshot script ```py # %% ctk_structure_screenshots.py import os import urllib.request from time import sleep import crystal_toolkit.components as ctc import dash from crystal_toolkit.core.plugin import CrystalToolkitPlugin from dash import dcc, html from dash.dependencies import Input, Output, State from pymatgen.core import Lattice, Structure __author__ = "Janosh Riebesell" __date__ = "2023-10-22" module_dir = os.path.dirname(__file__) os.makedirs(OUT_PATH := f"{module_dir}/screenshots", exist_ok=True) struct = Structure( lattice=Lattice.cubic(3), species=("Fe", "Fe"), coords=((0, 0, 0), (0.5, 0.5, 0.5)), ) structures = [struct] * 2 # %% struct_comp = ctc.StructureMoleculeComponent( show_compass=False, bonded_sites_outside_unit_cell=True, scene_settings={ "zoomToFit2D": False, "defaultZoom": 0.8, "enableZoom": True, "staticScene": False, "secondaryObjectView": False, }, ) layout = html.Div( [struct_comp.layout(), html.Div(id="temp_id"), dcc.Store(id="struct-id")] ) app = dash.Dash(plugins=[CrystalToolkitPlugin(layout=layout)]) @app.callback( Output(struct_comp.id(), "data"), Output("struct-id", "data", allow_duplicate=True), Input("struct-id", "data"), prevent_initial_call=True, ) def trigger_data_load(id): mat_id, struct = next(structures) return mat_id, struct @app.callback( Output(struct_comp.id("scene"), "imageRequest"), Input(struct_comp.id("graph"), "data"), ) def trigger_image_request(data): sleep(0.1) return {"filetype": "png"} @app.callback( Output("temp_id", "children"), Input(struct_comp.id("scene"), "imageDataTimestamp"), State(struct_comp.id("scene"), "imageData"), State("struct-id", "data"), ) def save_image(data, image_data, material_id): if material_id == "starting callback chain...": return material_id if image_data: response = urllib.request.urlopen(image_data) with open(f"{OUT_PATH}/{material_id}.png", "wb") as file: file.write(response.file.read()) return material_id app.run(port=8051, debug=True) ```
mkhorton commented 11 months ago

At the risk of asking the obvious, what version of dash_mp_components do you have installed? I see the release on Oct 23rd, did this include your most recent changes?

janosh commented 11 months ago

Yes, @yang-ruoxi made the 0.4.38 release (which I'm using) right after the new mp-react-components release that includes https://github.com/materialsproject/mp-react-components/pull/699.

yang-ruoxi commented 11 months ago

@janosh One still needs to add the prop in the corresponding component in dash-mp-component by hand, which was overlooked in the latest release.

janosh commented 11 months ago

Ah, I wasn't aware. Is that easy for you to do? If not, I'll take a look later.

yang-ruoxi commented 11 months ago

it's as simple as adding a prop in the corresponding component but it may take a bit before I can get to it. Please feel free to do it if it's time sensitive!

mkhorton commented 11 months ago

Ah, yes, non-obvious! Thanks @yang-ruoxi for explaining.

For additional context: I'm not sure if tooling has improved since this was set-up, but essentially the Dash component is a wrapper around the originating React component, and these are more or less equivalent except that the Dash component cannot have any non-JSON-serializable props (e.g., cannot have a JavaScript event prop). This has meant that the Dash component wrapper has to be written manually to make sure all props are supported types. Presumably this is something that could be automated, so maybe there's an easier way to do it now.

For even more context, both repos (mp-react-components and dash-mp-components) used to be within the Crystal Toolkit repo, but were split off just to make it a bit more obvious what each part does, and to make setting up the releases in CI a little easier. I'm still not sure if this was the right call, in some ways having them all in the same repo was simpler.

janosh commented 11 months ago

I think long-term it would be best to swap out the React layer entirely for something more modern like Svelte (and yes, have everything in 1 repo).

mkhorton commented 11 months ago

If this option had been available when Crystal Toolkit was first made, I'd have considered just using this instead: I'm sure it has some drawbacks (it looks like it can't use JSX, and I'm not sure how caching would work), but it would have made development much more rapid. We could still explore this for developing future components.

I think long-term it would be best to swap out the React layer entirely for something more modern like Svelte (and yes, have everything in 1 repo)

Does Svelte play nicely with Dash? The main benefit of Dash was calling underlying Python libraries directly (e.g., pymatgen functionality) without having to reimplement in JavaScript (painful, bug prone) or without having to write custom APIs for every pymatgen function etc. (tedious and verbose).

Again, however, if options like PyScript or Pyodide were available or more mature when Crystal Toolkit was written, perhaps we would have forgone Dash completely. With that said, I still really like Dash from the point of view of empowering researchers who are not web developers to rapidly create their own apps, so I'd like to see it stick around.