LRU raises #48

ncclementi · 2021-11-23T21:16:35Z

Based on #47
Closes dask/distributed#5364
This PR adds LRU to catch exceptions raised during a callback.

Test added/passes

This is ready for review.
cc: @jrbourbeau @crusaderky

zict/buffer.py

zict/lru.py

zict/tests/test_lru.py

crusaderky · 2021-11-30T12:06:03Z

zict/tests/test_lru.py

+    assert L == [("x", 1)]  # tried to evict and raised exception
+    assert lru.total_weight == 3
+    assert lru.weights == {"x": 1, "y": 1, "z": 1}
+


Please add check (black box preferably) that y and z are safely stored in self.d (suggestion: you can do it in the test above by setting weight=lambda k, v: v)

Please add check (black box preferably) that the heap hasn't been corrupted

Please add check (black box preferably) that y and z are safely stored in self.d (suggestion: you can do it in the test above by setting weight=lambda k, v: v)

I added a separate test that uses wight since in LRU n will mean two different things depending on whether we provide weights or not.

n: int Number of elements to keep, or total weight if weight= is used

I'm not sure if I capture what you meant by "check that y and z are stored safely. " let me know if I misunderstood you.

Please add check (black box preferably) that the heap hasn't been corrupted

I wasn't quite sure what you meant by "corrupted" here, I'm checking now that the keys are in the heap with the right priority.

Please test use case where individual weight >= n

I added a case to check weight >= n

All changes are in cab1041

crusaderky · 2021-11-30T12:06:31Z

zict/tests/test_lru.py

+    assert lru.total_weight == 3
+    assert lru.weights == {"x": 1, "y": 1, "z": 1}
+
+


Please test use case where individual weight >= n

Missing tests for Buffer

Thanks for pointing this out, I'll be adding buffer tests soon.

Co-authored-by: crusaderky <[email protected]>

zict/tests/test_lru.py

Co-authored-by: crusaderky <[email protected]>

ncclementi · 2021-12-03T19:56:35Z

zict/tests/test_buffer.py

+    # fail raise we might want to avoid), not getting to the callbacks.
+    # If I do try/except and pass in slow_to_fast things are handled well, but not sure the
+    # pass is the right call here.
+    assert s2f == [


Tests are broken because there is one small thing I was not quite sure how to fix. It's explained on the commented code. I wonder what's the best approach for this,

To answer your question you should first have a legitimate use case where the buffer callback raises, and I cannot think of any. So I don't think we should try doing the right thing when we don't even know what "right" looks like.
There's also a wealth of potential issues when there is a chain of multiple callbacks and the first one fails, leaving the rest unexecuted. What does it mean? We don't know, it entirely depends on the callbacks. Again, we should not worry about it.

This whole PR is specifically to deal with Buffer.slow.__setitem__ raising - let's not get off track.

setup.py

zict/__init__.py

zict/tests/test_buffer.py

crusaderky · 2021-12-07T11:56:49Z

zict/tests/test_buffer.py

+    # fail raise we might want to avoid), not getting to the callbacks.
+    # If I do try/except and pass in slow_to_fast things are handled well, but not sure the
+    # pass is the right call here.
+    assert s2f == [


To answer your question you should first have a legitimate use case where the buffer callback raises, and I cannot think of any. So I don't think we should try doing the right thing when we don't even know what "right" looks like.
There's also a wealth of potential issues when there is a chain of multiple callbacks and the first one fails, leaving the rest unexecuted. What does it mean? We don't know, it entirely depends on the callbacks. Again, we should not worry about it.

This whole PR is specifically to deal with Buffer.slow.__setitem__ raising - let's not get off track.

Co-authored-by: crusaderky <[email protected]>

crusaderky · 2021-12-10T11:31:08Z

@jrbourbeau please merge

jrbourbeau · 2021-12-14T15:45:38Z

zict/buffer.py

+        try:
+            for cb in self.fast_to_slow_callbacks:
+                cb(key, value)
+        # LRU catches exception, raises and makes sure keys are not lost and located in fast.


Thanks for working on this @ncclementi and reviewing @crusaderky. @ncclementi can you add a short description of this behavior change to the Buffer and LRU docstrings? For both classes, a description along the lines of "If an exception occurs during X then Y happens." in the appropriate parameter description (e.g. on_evict for LRU) should be sufficient

Added the documentation to the appropriate parameters: on_evict (LRU) and fast_to_slow_callback (Buffer).

ncclementi · 2022-01-07T21:54:48Z

zict/lru.py

+            # e.g. if a callback tried storing to disk and raised a disk full error
+            self.heap[k] = priority
+            self.d[k] = v
+            raise


I think there is a problem with this raise. I was trying some things around where I ended up having on fast multiple keys as a result of disk_limit being reached, {'a': 400, 'd': 100, 'e': 500} . In slow after deleting a key I have only {'c': 200}, let's say the disk_limit = 500 . If I add a new key ('f': 100) that is small to go on fast, I'd expect 'd' that is on fast to be moved to slow. But this doesn't happen since the first key that it tries to move due to low priority is 'e' and it's too big that it raises and therefore it stops going over the rest of the dictionary, even though there is clearly room for 'd' on slow.

That's fine. The LRU does not and should not know that the reason for the exception is that the data is too large. It would be expensive and complicated to try writing the other keys just in case they do not fail. Also, I don't think the use case is very interesting in real life in distributed.

crusaderky · 2022-01-10T11:48:40Z

What is outstanding on this PR?

crusaderky · 2022-02-16T11:04:30Z

Thank you!

crusaderky and others added 2 commits November 19, 2021 10:14

LRU callbacks may raise

ef47d0b

catch exception on buffer setitem

00362df

ncclementi mentioned this pull request Nov 23, 2021

Constrained spill dask/distributed#5543

Merged

ncclementi changed the title ~~Slow raises and buffer exception~~ LRU raises and buffer exception Nov 23, 2021

crusaderky mentioned this pull request Nov 29, 2021

LRU callbacks may raise #47

Closed

ncclementi added 3 commits November 29, 2021 12:03

remove debugging code

6bc2b6b

remove print statement

10c8942

add test for callback exception handling

f0ca20e

ncclementi changed the title ~~LRU raises and buffer exception~~ LRU raises Nov 29, 2021

ncclementi marked this pull request as ready for review November 29, 2021 22:19

ncclementi mentioned this pull request Nov 29, 2021

Spill to constrained disk space dask/distributed#5364

Closed

crusaderky requested changes Nov 30, 2021

View reviewed changes

ncclementi and others added 8 commits November 30, 2021 16:00

Add comment on setitem buffer

1858a24

Co-authored-by: crusaderky <[email protected]>

Add comment to LRU set handling

b5df778

Co-authored-by: crusaderky <[email protected]>

add comment on lru evict except

f36a76d

Co-authored-by: crusaderky <[email protected]>

modify test_lru exception case

9e8d072

Co-authored-by: crusaderky <[email protected]>

rasie MyError instead of just Exception

4bb207d

Co-authored-by: crusaderky <[email protected]>

Update zict/tests/test_lru.py

362d5c7

Co-authored-by: crusaderky <[email protected]>

Update zict/tests/test_lru.py

ba82bc2

Co-authored-by: crusaderky <[email protected]>

add test for weight

cab1041

crusaderky requested changes Dec 1, 2021

View reviewed changes

zict/tests/test_lru.py Outdated Show resolved Hide resolved

zict/tests/test_lru.py Outdated Show resolved Hide resolved

zict/tests/test_lru.py Show resolved Hide resolved

zict/tests/test_lru.py Outdated Show resolved Hide resolved

zict/tests/test_lru.py Outdated Show resolved Hide resolved

ncclementi and others added 6 commits December 1, 2021 17:46

Apply suggestions from code review

343683e

Co-authored-by: crusaderky <[email protected]>

Apply suggestions from code review

0cb5fce

Co-authored-by: crusaderky <[email protected]>

fix lru exception weight test

625ef71

being consitent with raises

c11f5f8

let LRU habndle exception when raises happen on buffer

8b59c59

add test for buffer (broken)

f2d40e4

ncclementi commented Dec 3, 2021

View reviewed changes

tempary version bump for testing distributed

f1d8c9e

crusaderky requested changes Dec 7, 2021

View reviewed changes

ncclementi and others added 5 commits December 8, 2021 11:52

Update setup.py

6991dc9

Co-authored-by: crusaderky <[email protected]>

Update zict/__init__.py

bbb1a95

Co-authored-by: crusaderky <[email protected]>

Update zict/tests/test_buffer.py

b963614

Co-authored-by: crusaderky <[email protected]>

Update zict/tests/test_buffer.py

357970d

Co-authored-by: crusaderky <[email protected]>

remove confusing part of test (no need)

2401ec4

crusaderky approved these changes Dec 10, 2021

View reviewed changes

jrbourbeau reviewed Dec 14, 2021

View reviewed changes

ncclementi added 2 commits December 14, 2021 11:44

add exception behaviour to docstrings of lru and buffer

7631d70

Be more explicit about documentation on exceptions

78aab6b

ncclementi commented Jan 7, 2022

View reviewed changes

fix version to test distributed

2252a0e

crusaderky merged commit e1886a4 into dask:main Feb 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LRU raises #48

LRU raises #48

ncclementi commented Nov 23, 2021 •

edited by crusaderky

Loading

crusaderky Nov 30, 2021 •

edited

Loading

ncclementi Dec 1, 2021

crusaderky Nov 30, 2021

ncclementi Dec 1, 2021

ncclementi Dec 3, 2021

crusaderky Dec 7, 2021

crusaderky Dec 7, 2021

crusaderky commented Dec 10, 2021

jrbourbeau Dec 14, 2021

ncclementi Dec 14, 2021 •

edited

Loading

ncclementi Jan 7, 2022 •

edited

Loading

crusaderky Jan 10, 2022

crusaderky commented Jan 10, 2022

crusaderky commented Feb 16, 2022

		assert lru.total_weight == 3
		assert lru.weights == {"x": 1, "y": 1, "z": 1}

LRU raises #48

LRU raises #48

Conversation

ncclementi commented Nov 23, 2021 • edited by crusaderky Loading

crusaderky Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky commented Dec 10, 2021

Choose a reason for hiding this comment

ncclementi Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

ncclementi Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky commented Jan 10, 2022

crusaderky commented Feb 16, 2022

ncclementi commented Nov 23, 2021 •

edited by crusaderky

Loading

crusaderky Nov 30, 2021 •

edited

Loading

ncclementi Dec 14, 2021 •

edited

Loading

ncclementi Jan 7, 2022 •

edited

Loading