allow use of validate_all=True on nested evented models

[brisvag]

Description

During my explorations for nested evented models, I found a solution for what #4138 tried to solve, but which allows us to keep validate_all = True on evented models. On suggestion of @tlambert03, I'm splitting it out of #4522 to get it merged indepentently.

Type of change

• Bug-fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

References

How has this been tested?

• example: the test suite for my feature covers cases x, y, and z
• example: all tests pass with my change

Final checklist:

• My PR is the minimum possible work for the desired functionality
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• If I included new strings, I have used trans. to make them localizable.
  For more information see our translations guide.

[tlambert03]

very nice solution

[brisvag]

Turns out runtime_checkable Protocols are not powerful enough for this application, due to type hints not being accounted for, so I'm falling back to a simple instance check.

[Czaki]

Turns out runtime_checkable Protocols are not powerful enough for this application, due to type hints not being accounted for, so I'm falling back to a simple instance check.

but you could just simple overwrite __instancecheck__

[brisvag]

Turns out runtime_checkable Protocols are not powerful enough for this application, due to type hints not being accounted for, so I'm falling back to a simple instance check.

but you could just simple overwrite __instancecheck__

Yup, I thought about that and tried... And then I learned that instancecheck needs to be on the metaclass, and suddenly things became complicated xD If you have some ideas on how to make it work, please let me know! I really got stuck when I had to somehow change the metaclass of the decorated class...

[Czaki]

@brisvag did this code is enough: ?

from abc import ABCMeta
from napari.utils.events import EmitterGroup


class EventedMetaclass(ABCMeta):
    def __instancecheck__(self, instance):
        return (
            hasattr(instance, "events")
            and isinstance(instance.events, EmitterGroup)
        )

class Evented(metaclass=EventedMetaclass):
    events: EmitterGroup


class SampleEvented:
    def __init__(self):
        self.events = EmitterGroup(added=None, removed=None)


class SampleNonEvented:
    def __init__(self):
        self.events = 1

assert isinstance(SampleEvented(), Evented)
assert not isinstance(SampleNonEvented(), Evented)

[brisvag]

Oh, that's cool! I was trying to build on top of Protocol, but this is a lot more straight forward. I'll play around with it!

[brisvag]

Question: how do typical IDEs use Protocols for static typing? Can we actually just use a completely different class such as above and get the same benefits? Or should we subclass Protocols so they can use it? Or maybe is _ProtocolMeta enough?

Either way, I think this might be useful for the future, but not worth holding up this PR for. This is functional as is and pretty straight forward!

[codecov]

Codecov Report

Merging #4551 (f8a2833) into main (1546f18) will increase coverage by 1.76%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4551      +/-   ##
==========================================
+ Coverage   86.05%   87.81%   +1.76%     
==========================================
  Files         585      585              
  Lines       50073    50098      +25     
==========================================
+ Hits        43089    43996     +907     
+ Misses       6984     6102     -882     

Impacted Files	Coverage Δ
napari/utils/events/evented_model.py	94.03% <100.00%> (+0.33%)	⬆️
napari/plugins/pypi.py	45.61% <0.00%> (-0.82%)	⬇️
napari/utils/config.py	100.00% <0.00%> (ø)
napari/layers/utils/string_encoding.py	100.00% <0.00%> (ø)
napari/components/experimental/monitor/_api.py	0.00% <0.00%> (ø)
napari/utils/_tests/test_key_bindings.py	96.49% <0.00%> (+0.02%)	⬆️
napari/layers/utils/_color_encoding.py	93.75% <0.00%> (+0.06%)	⬆️
napari/layers/utils/style_encoding.py	96.26% <0.00%> (+0.07%)	⬆️
napari/layers/shapes/_tests/test_shapes_utils.py	97.22% <0.00%> (+0.07%)	⬆️
napari/__main__.py	54.72% <0.00%> (+0.22%)	⬆️
... and 63 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1546f18...f8a2833. Read the comment docs.

[sofroniewn]

Wow - just came across this from #4522 - super clever @brisvag if I understand this right.

Agreed this should go in before #4522 and might make that diff a little smaller and easier to review.

Should this PR also contain the changes related to

this also lets us stop using those very verbose Field(default_factory..., allow_mutation=False,), combined with maunally "validated" defaults because of validate_all = False, by triggering a presudo-validation after init.

that will enable us

to do Viewer.camera = new_camera without messing up viewer.events.camera and viewer.camera.events.

That can come later, but I believe will be supported effectively now.

One things is that it would be nice to have some tests for this new behaviour, something like

# Note this may not work exactly - but is meant to be illustrative
def test_events_stay_working():
    class SimpleCamera(EventedModel):
        zoom: int

    class SimpleViewer(EventedModel):
        camera: SimpleCamera

    mock_callback = Mock()

    # Create simple viewer object
    simple_viewer = SimpleViewer(camera=SimpleCamera(zoom=2))
    # Connect camera zoom to some callback function
    simple_viewer.camera.events.zoom.connect(mock_callback)
    # Create a new camera
    new_camera = SimpleCamera(zoom=3)
   # Replace original camera with new camera - update should now be in place
    simple_viewer.camera = new_camera
   # Update simple_viewer camera zoom with new value
    simple_viewer.camera.zoom = 4
    # Check that original callback still gets called with new zoom value
    mock_callback.method.assert_called_once(value=4)

If I have it right that this behaviour will be enabled by this PR

[brisvag]

If I have it right that this behaviour will be enabled by this PR

Not quite, unfortunately. for that to work, we need the machinery from #4522 that makes inplace mutation possible.

I think it's possible to split that out of #4522... but I need to think about it :P

[sofroniewn]

If I have it right that this behaviour will be enabled by this PR

Not quite, unfortunately. for that to work, we need the machinery from #4522 that makes inplace mutation possible.

I think it's possible to split that out of #4522... but I need to think about it :P

Ah ok, well I think fine to still merge this as is then with validate_all = True just being a net good thing.

I think it might be nice if you could isolate that logic from #4522 and create a new standalone PR for it with a test like i described.

I will let you or @tlambert03 merge this one when you are ready

[brisvag]

I just found out that this is not enough, because the field events of the children also need to be updated, so we need a recursive version of this.

In [1]: import napari

In [2]: v = napari.Viewer(ndisplay=3)

In [3]: v.camera.events.zoom.source is v.camera
Out[3]: False

[brisvag]

Fixed it with a recursive methods:

In [1]: import napari

In [2]: v = napari.Viewer(ndisplay=3)

In [3]: v.camera.events.zoom.source is v.camera
Out[3]: True

[andy-sweet]

@brisvag : not sure where this fits with the related open PRs, but it's relatively old and doubly-approved. Therefore, it would be good to merge this or mark it as draft/wip if it's likely to be superseded by another.

FYI, I think if you merge main most of the checks should pass, give or take some luck on the flaky tests.

[brisvag]

I think this can be merged regardless of the rest, but I'd be more comfortable with an extra confirmation from @tlambert03 in light of #4609, just to make sure I'm not doing anything here that somehow assumes the changes there and I'm not realizing :)

[tlambert03]

Good by me! feel free to merge after tests pass