Add support for Marimo notebooks - #73
Conversation
…d adjust core utilities.
…ning `vstack` with the widget and content.
|
Thanks @N283T for making a PR, this is greatly appreciated! Coincidentally, I was planning to add direct marimo support in a refactoring branch #71, but until I manage to get more time to work on this I think we should go ahead with your PR. I'll enable the CI pipelines, if you could address any issues highlighted there that would be great, and I've already added some minor comments (the main one being that we don't want marimo to be required when installing mols2grid). If you need any help with the above I'm happy to go into more details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #73 +/- ##
==========================================
+ Coverage 90.20% 90.86% +0.65%
==========================================
Files 8 8
Lines 531 558 +27
==========================================
+ Hits 479 507 +28
+ Misses 52 51 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cbouy
left a comment
cbouy
left a comment
There was a problem hiding this comment.
Sorry I just realised I never actually posted my PR comments, here they are
| if is_running_within_marimo(): | ||
| import marimo as mo | ||
|
|
||
| if use_iframe: | ||
| # Render HTML in iframe. | ||
| iframe = env.get_template("html/iframe.html").render( | ||
| width=iframe_width, | ||
| height=iframe_height, | ||
| allow=iframe_allow, | ||
| sandbox=iframe_sandbox, | ||
| doc=escape(doc), | ||
| ) | ||
| return mo.vstack([self.widget, mo.Html(iframe)]) | ||
| return mo.vstack([self.widget, mo.Html(doc)]) |
There was a problem hiding this comment.
this shouldn't repeat the use_iframe block below, instead do somethiing like this to keep it DRY:
requires_marimo = is_running_within_marimo()
if use_iframe or requires_marimo:
iframe = env.get_template...
if requires_marimo:
import marimo as mo
return mo.vstack...
return HTML(iframe)
return HTML(doc)| [project.optional-dependencies] | ||
| marimo = ["marimo>=0.18.4"] | ||
|
|
There was a problem hiding this comment.
no need to declare it as an optional dependency either, move to tests under [dependency-groups] below
|
you should be able to fix the ruff error with |
…dependency configuration.
|
Thanks for the review! I've addressed the comments:
Additionally, |
|
Looks good, thanks @N283T ! Some additional small things and then I'll merge:
solubility_range = mo.ui.range_slider(
-10, 2, 0.5, debounce=True, show_value=True, full_width=True, label="Solubility"
)
grid = mols2grid.MolGrid.from_sdf(mols2grid.datafiles.SOLUBILITY_SDF, size=(120, 100))
view = grid.display(n_items_per_page=12)results = grid.dataframe.loc[
(grid.dataframe["SOL"] >= solubility_range.value[0]) & (grid.dataframe["SOL"] <= solubility_range.value[1])
]
grid.filter_by_index(results.index)
mo.vstack([solubility_range, view])This will make it easier for me to test that all relevant functionalities are preserved when I make changes in the future |
|
Thanks! I’ve updated the example following the suggested snippet:
Note that grid initialization needs to be kept in a separate cell: in marimo, slider updates trigger re-execution of dependent cells, and recreating the grid on each update would reset the widget state. |
|
Unrelated follow-up (but still marimo-related):
I have a local implementation of a small The helper looks roughly like this: def get_selection_state(self):
if not is_running_within_marimo():
raise RuntimeError("This method is only available in a marimo notebook.")
import marimo as mo
get_state, set_state = mo.state([])
def _on_change(change):
try:
sel = ast.literal_eval(change["new"])
set_state(list(sel.keys()))
except (ValueError, SyntaxError):
pass
if not getattr(self.widget, "_marimo_hooked", False):
self.widget.observe(_on_change, names=["selection"])
self.widget._marimo_hooked = True
return get_stateWith this in place, an example marimo workflow looks like: ...
# display grid
mo.vstack(
[solubility_range, view]
)# display table
selected = results[results["mols2grid-id"].isin(sel_ids())]
table = mo.ui.table(
selected.reset_index(drop=True).drop(columns="img"),
format_mapping={"mol": mol_to_svg},
freeze_columns_left=["mols2grid-id", "mol"],
freeze_columns_right=["SOL"],
label="Selected Molecules"
)
table
This helper is not included in the current PR, but if you think it would be
Just let me know which you prefer. |
|
Sorry, I accidentally pushed a middle state earlier. |
|
That looks like a great addition, thanks for working on that! |
…date the Marimo example to display selected molecules.
DescriptionThis PR adds first-class support for reactive molecule selection in marimo by introducing a small helper on Changes
Notes
|
cbouy
left a comment
cbouy
left a comment
There was a problem hiding this comment.
Really nice addition! Just some minor comments, don't forget to run uv run poe style-fix
| else: | ||
| self._cached_selection = {} | ||
| register._init_grid(name) | ||
| if is_running_within_marimo(): |
There was a problem hiding this comment.
it would be better to directly skip emitting the warning in _init_grid here instead of catching warnings:
if overwrite and not is_running_within_marimo():| df = pd.DataFrame({"SMILES": ["C"]}) | ||
| mg = MolGrid(df, smiles_col="SMILES") |
There was a problem hiding this comment.
might as well set this up as a fixture at the top of the file since it's used in almost all tests here
| mg.get_selection_state() | ||
|
|
||
| # Verify observe was called | ||
| assert mock_observe.called |
There was a problem hiding this comment.
nitpick: mock_observe.assert_called()
| def _on_change(change): | ||
| try: | ||
| sel = ast.literal_eval(change["new"]) | ||
| set_state(list(sel.keys())) | ||
| except (ValueError, SyntaxError): | ||
| pass |
There was a problem hiding this comment.
Just me thinking out loud, definitely NOT something to address in this PR, but in my refactoring branch,
I might move this to be directly integrated in the SelectionRegister.selection_updated in select.py with some mechanism to register custom callbacks that would be triggered by selection_updated, just so that we don't have multiple observers listening to the same event, e.g.
register.add_callback(lambda sel: set_state(list(sel.keys())))| columns=self._extra_columns | ||
| ) | ||
|
|
||
| def get_selection_state(self): |
There was a problem hiding this comment.
since this is specific to marimo, it should have a more descriptive name, something like marimo_selection_getter?
| # Same as: | ||
| # results = grid.dataframe.loc[ | ||
| # (grid.dataframe["SOL"] >= solubility_range.value[0]) & (grid.dataframe["SOL"] <= solubility_range.value[1]) | ||
| # ] |
There was a problem hiding this comment.
should be moved one line up as:
| # Same as: | |
| # results = grid.dataframe.loc[ | |
| # (grid.dataframe["SOL"] >= solubility_range.value[0]) & (grid.dataframe["SOL"] <= solubility_range.value[1]) | |
| # ] | |
| # Same as: | |
| # grid.dataframe["SOL"] >= solubility_range.value[0]) & (grid.dataframe["SOL"] <= solubility_range.value[1]) |
| view = grid.display(n_items_per_page=12) | ||
| return df, grid, view | ||
| grid = mols2grid.MolGrid.from_sdf(SOLUBILITY_SDF, size=(120, 100)) | ||
| sel_ids = grid.get_selection_state() |
There was a problem hiding this comment.
nitpick:
| sel_ids = grid.get_selection_state() | |
| get_selection_ids = grid.get_selection_state() |
| ".vscode", | ||
| "build", | ||
| "site-packages", | ||
| "scripts/marimo_example.py", |
There was a problem hiding this comment.
Please revert, I'd rather not ignore formatting/linting without a good reason 😅
…ning logic and renaming selection getter.
|
Thanks for the review! Sorry for the bundled reply — I addressed all the comments in one go:
Also ran |
|
All good from my side! Could you just add something in the |
|
Thanks! I've added a short note about Marimo compatibility in |
cbouy
left a comment
cbouy
left a comment
There was a problem hiding this comment.
All good, thanks a ton @N283T !
I'm going to make the release today and announce it on socials once the conda build is done (most likely tomorrow), should I add a link to your GitHub account, or is there a Linkedin/X/Bluesky/personal website link that you'd prefer?
|
Thanks a lot! Once it’s released, I’ll share it in Japanese and help spread the word in the Japanese community as well !! |
2 participants
Description
This PR adds support for running
mols2gridwithin marimo notebooks.Previously, the grid would not render correctly or support interactivity in marimo due to differences in how JavaScript, HTML, and widgets are handled compared to Jupyter. This implementation fixes the issue by:
iframewhen in marimo. This isolates the grid's styles and scripts, ensuring it displays correctly.marimo.vstackcontaining both theanywidgetinstance and themarimo.Htmlgrid. This ensures theanywidgetbackend is active, enabling bidirectional communication (e.g., for selection).__init__. In Marimo, we manually return the widget indisplay(), so the automatic display from__init__(used in Jupyter) caused double rendering and warnings.Changes
mols2grid/utils.py: Addedis_running_within_marimo()utility function.mols2grid/molgrid.py:MolGrid.display()to usemarimo.Htmlin an iframe and return it wrapped inmarimo.vstackwith the widget.MolGrid.__init__to skipdisplay(widget)when running in marimo to prevent double rendering.tests/test_marimo_integration.py: Added unit tests to verify marimo detection and display logic.Verification
mols2grid.display(df)renders the interactive grid correctly in a local marimo notebook.mols2grid.get_selection().uv run pytest.