Skip to content
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

use_memo should warn/throw if passed anything but an array for dependencies #355

Closed
mattrunyon opened this issue Mar 12, 2024 · 4 comments · Fixed by #356
Closed

use_memo should warn/throw if passed anything but an array for dependencies #355

mattrunyon opened this issue Mar 12, 2024 · 4 comments · Fixed by #356
Assignees
Labels
deephaven.ui enhancement New feature or request

Comments

@mattrunyon
Copy link
Collaborator

mattrunyon commented Mar 12, 2024

Related to #354 where we accidentally had a dictionary instead of an array for the dependencies of use_memo. We should at least warn (maybe throw) that memoization will not happen properly if a non-array is passed

I also don't think the dependencies typehint is right. set[Any] | Sequence[Any]. Sets are unordered, so you could cache miss just based on set iteration order. Sequences contain list, tuple, and range. Range doesn't really make sense IMO. List vs tuple I could see either being valid. From what I can find, tuples are immutable and a little bit more performant than lists

@mofojed
Copy link
Member

mofojed commented Mar 12, 2024

@mattrunyon it wasn't a dict, it was a Set, which we allow as a type. @jnumainville maybe we shouldn't allow a Set though (as that bug in #354 is kind of subtle)

@mattrunyon
Copy link
Collaborator Author

mattrunyon commented Mar 12, 2024

Ya I just updated my comment here. It should just be List or maybe List | Tuple. Sequence includes other data types which we don't want. Set is unordered and definitely shouldn't be allowed considering the comparison is just based on position in the array

@vbabich vbabich added question Further information is requested and removed triage labels Mar 12, 2024
@mofojed
Copy link
Member

mofojed commented Mar 12, 2024

After discussion, definitely should not be allowing sets - the order definitely matters, e.g.

@ui.component
def Foo():
  val1, setValue1 = use_state(0)
  val2, setValue2 = use_state(0)
  
  # These values are the same! Should be using an order so the slot they're in matters.
  new_val = use_memo(lambda:...., { val1, val2 })

@jnumainville
Copy link
Collaborator

ah, yeah, a set is too permissive and would be too much hassle to make work even if it didn't have the issue of dropping duplicate items

@mofojed mofojed removed the question Further information is requested label Mar 12, 2024
jnumainville added a commit that referenced this issue Mar 13, 2024
Fixes #355 
Only allow tuple and list, and throw if neither.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deephaven.ui enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants