Skip to content

Avoid ValueError when comparing embed defs with NumPy arrays #7980

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.

Sign up for GitHub

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

Jump to bottom
Merged
mscolnick merged 4 commits into from
Jan 27, 2026
Merged

Avoid ValueError when comparing embed defs with NumPy arrays #7980

mscolnick merged 4 commits into from
Jan 27, 2026
+89 −1

Conversation

@MukeshK17
Copy link
Contributor

@MukeshK17 MukeshK17 commented Jan 26, 2026

Fix embed caching when defs contain NumPy arrays

Summary

Fixes a bug where App.embed() could raise a ValueError when embed defs contain NumPy arrays.
The issue occurred during cache checks when comparing defs using ==, which is ambiguous for NumPy arrays.

Problem

Changing embed defs containing NumPy arrays (e.g. arrays with different shapes) could raise:

ValueError: The truth value of an array with more than one element is ambiguous

This originated in AppKernelRunner.are_outputs_cached.

Solution

  • Added a safe helper (_defs_equal) for comparing embed defs
  • Uses numpy.array_equal for NumPy arrays
  • Falls back safely for ambiguous comparisons
  • Updated cache logic to use this helper

Tests

Added regression tests ensuring:

  • Different NumPy arrays invalidate the cache
  • Equal NumPy arrays reuse the cache

These tests fail on main and pass with this change.

Fixes #7969

CLA

I have read the CLA Document and I hereby sign the CLA.
@MukeshK17 MukeshK17 requested a review from dmadisetti as a code owner January 26, 2026 20:25
@vercel
Copy link

vercel bot commented Jan 26, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
marimo-docs Ready Ready Preview, Comment Jan 27, 2026 7:41am

Request Review
@github-actions
Copy link

github-actions bot commented Jan 26, 2026 •
edited
Loading

All contributors have signed the CLA ✍️ ��
Posted by the CLA Assistant Lite bot.
@MukeshK17
Copy link
Contributor Author

MukeshK17 commented Jan 26, 2026

I have read the CLA Document and I hereby sign the CLA
mscolnick
mscolnick reviewed Jan 26, 2026
View reviewed changes
tests/_runtime/runner/test_kernel_runner.py
def test_numpy_defs_do_not_crash_and_invalidate_cache():
runner = _make_runner()

import numpy as np
Copy link
Contributor

@mscolnick mscolnick Jan 26, 2026

Choose a reason for hiding this comment

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

we will need to skip these tests if numpy is not installed.

you can do @pytest.mark.requires("numpy") on the test function
Copy link
Contributor Author

@MukeshK17 MukeshK17 Jan 26, 2026

Choose a reason for hiding this comment

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

Thanks for the feedback! I’ve updated the code as requested.
mscolnick
mscolnick reviewed Jan 26, 2026
View reviewed changes
marimo/_runtime/app/kernel_runner.py Outdated

try:
# numpy-safe comparison
try:
Copy link
Contributor

@mscolnick mscolnick Jan 26, 2026

Choose a reason for hiding this comment

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

can you do if DependencyManager.numpy.imported() before this so we don't import numpy if it hasnt been imported (otherwise this could be a slower operation
Copy link
Contributor Author

@MukeshK17 MukeshK17 Jan 26, 2026

Choose a reason for hiding this comment

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

I’ve updated the implementation to use DependencyManager.numpy as suggested.
@mscolnick mscolnick requested a review from manzt January 26, 2026 20:34
mscolnick
mscolnick previously approved these changes Jan 26, 2026
View reviewed changes
@mscolnick
Copy link
Contributor

mscolnick commented Jan 26, 2026

@MukeshK17 there is a small typecheck error

marimo/_runtime/app/kernel_runner.py:50: error: Incompatible types in assignment (expression has type "None", variable has type Module) [assignment]
@MukeshK17
Copy link
Contributor Author

MukeshK17 commented Jan 27, 2026

Fixed the reported typecheck issue.
@MukeshK17 MukeshK17 requested a review from mscolnick January 27, 2026 07:47
@mscolnick mscolnick added the bug Something isn't working label Jan 27, 2026
@mscolnick mscolnick merged commit 50642e9 into marimo-team:main Jan 27, 2026
41 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

2 participants

@MukeshK17 @mscolnick