feat: use custom root type for KeyBinding #65

kne42 · 2022-09-29T21:09:09Z

Use a custom root type so that models_as_dict=False does not need to be passed to the .json() method since this parameter does not get passed recursively along models. This changes the .dict() method to replace KeyBinding with its __root__, which is a str, thus effectively having the data represented as a string for serialization purposes while still being able to access the rest of the functionality attached to the KeyBinding class.

If I have implemented this correctly, there should be no effective changes to the API.

codecov · 2022-09-29T21:10:53Z

Codecov Report

Merging #65 (e524085) into main (817a1b1) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
- Coverage   99.34%   99.30%   -0.05%     
==========================================
  Files          31       31              
  Lines        1692     1729      +37     
==========================================
+ Hits         1681     1717      +36     
- Misses         11       12       +1

Impacted Files	Coverage Δ
src/app_model/types/_keys/_keybindings.py	`99.50% <100.00%> (-0.50%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

andy-sweet

I can't say I love the extra code + complexity here, but I also don't have any better ideas, so approving because the desired behavior is at least well tested.

I had one optional idea around mutability that might simplify things, but not sure if it's viable.

Also, to confirm, the reason we want KeyBinding serialized to a string (rather than a list of dicts) is because we want it to human-readable and modifiable in something like a JSON or YAML file?

andy-sweet · 2022-09-30T16:26:12Z

src/app_model/types/_keys/_keybindings.py

+        return self._parts
+
+    @no_type_check
+    def __setattr__(self, key, val):


Idea: do we need to support mutation of KeyBinding? This adds extra code to maintain, which is mostly just a repeat of __init__ anyway.

I don't think performance is a serious concern here right? Especially since parts is likely to remain the only member of this thing anyway.

If we made this model frozen/immutable, then we could just derive the parts property (likely as a cached property) and maybe add a factory method like KeyBinding.from_parts to avoid some of the complexity in __init__ too.

andy-sweet · 2022-09-30T16:27:01Z

tests/test_keybindings.py

@@ -100,6 +117,37 @@ def test_in_dict():
    assert kbs[hash(new_a)] == 0


+def test_errors():


Love the test coverage of error cases.

andy-sweet · 2022-09-30T16:43:04Z

src/app_model/types/_keys/_keybindings.py

+            errors=[ErrorWrapper(exc=exc, loc=("parts",))], model=cls
+        )
+
+    def __init__(self, **data: Any) -> None:


Optional: any strong reason to use **data instead of expanding the two known parameters __root__ and parts as named keyword arguments with typing info?

tlambert03 · 2022-09-30T20:24:29Z

I can't say I love the extra code + complexity here, but I also don't have any better ideas, so approving because the desired behavior is at least well tested.

I agree, while I also think the goal is reasonable (not having to pass models_as_dict to the json() method), I think what the implementation here does to the code is unfortunate.

I had one optional idea around mutability that might simplify things, but not sure if it's viable.

I think making this immutable is a great idea. In any case, even if it's not immutable, I dislike the duplicated logic in the __init__ and in setattr. (That's kind what @validator is there for in the first place... so call it in the __init__ if you need, rather duplicating its logic elswhere)

Also, to confirm, the reason we want KeyBinding serialized to a string (rather than a list of dicts) is because we want it to human-readable and modifiable in something like a JSON or YAML file?

This is also my main question here: while I can see that it's a nicer way to serialize it, can you point out where you ran into this problem? i.e. where you were trying to serialize it (rather than, say, using str(keybinding)) such that you found all this necessary?

(an alternative very simple solution here is to set the json encoder on the model that contains the keybinding, so it would be nice to know why/where that approach is insufficient)

tlambert03 · 2022-09-30T20:40:07Z

src/app_model/types/_keys/_keybindings.py

+        """Key combinations that make up the overall key chord."""
+        return self._parts
+
+    @no_type_check


please type stuff in this library

tlambert03 · 2022-09-30T20:57:15Z

if you do want to change the root type of KeyBinding to str (which I'm not convinced is necessary/advisable) then I think that parts should probably just be made a property:

    __root__: str

    @property
    def parts(self) -> List[SimpleKeyBinding]:
        # could cache this and make keybinding immutable
        return [SimpleKeyBinding.from_str(part) for part in self.__root__.split()]

(and then get rid of all the __init__ and __setattr__ logic... eg main...tlambert03:app-model:alt)

kne42 · 2022-09-30T21:20:44Z

This is also my main question here: while I can see that it's a nicer way to serialize it, can you point out where you ran into this problem? i.e. where you were trying to serialize it (rather than, say, using str(keybinding)) such that you found all this necessary?

(an alternative very simple solution here is to set the json encoder on the model that contains the keybinding, so it would be nice to know why/where that approach is insufficient)

You can find the issue I ran into in this Zulip thread

Here's a simple example you can run yourself if you're not convinced:

In [1]: from pydantic import BaseModel

In [2]: class Client(BaseModel):
   ...:     title: str
   ...:     first: str
   ...:     last: str
   ...:

In [3]: class Lawyer(BaseModel):
   ...:     client: Client
   ...:
   ...:     class Config:
   ...:         json_encoders = {
   ...:             Client: lambda c: f'{c.title} {c.first} {c.last}'
   ...:         }
   ...:

In [4]: class Office(BaseModel):
   ...:     lawyer: Lawyer
   ...:
   ...:     class Config:
   ...:         json_encoders = {
   ...:             Client: lambda c: f'{c.title} {c.first} {c.last}'
   ...:         }
   ...:

In [5]: client = Client(title='Dr.', first='Medikal', last='School')

In [6]: client.json(models_as_dict=False)
Out[6]: '{"title": "Dr.", "first": "Medikal", "last": "School"}'

In [7]: lawyer = Lawyer(client=client)

In [8]: lawyer.json(models_as_dict=False)
Out[8]: '{"client": "Dr. Medikal School"}'

In [9]: office = Office(lawyer=lawyer)

In [10]: office.json(models_as_dict=False)
Out[10]: '{"lawyer": {"client": {"title": "Dr.", "first": "Medikal", "last": "School"}}}'

tlambert03 · 2022-09-30T21:35:41Z

I don't need convincing of pydantic's behavior :) I'm aware of it/your previous examples were convincing enough

I guess, I'm kinda asking where exactly in your code you needed this. I did read the zulip thread and don't see an application there, just more MREs... and a link to the github issue where you said:

There's an issue serializing KeyBinding to json, since to properly use the json_encoders config settings on other models, you must pass models_as_dict=False (see pydantic/pydantic#3542). However, this aspect doesn't seem to be passed between json calls, so neither overriding the method to change the default on ShortcutSettings nor calling get_settings().json(models_as_dict=False) fixes the problem.

but what I'm looking for is the literal place where you're using keybinding, would you mind linking it? (i.e. whats your Office in the example above, and can you not add a json_encoder for Lawyer in addition to Client):

def enc_client(c: 'Client'):
    return f"{c.title} {c.first} {c.last}"

class Office(BaseModel):
    lawyer: Lawyer

    class Config:
        json_encoders = {Client: enc_client, Lawyer: lambda l: {'client': enc_client(l.client)}}

kne42 · 2022-10-01T15:42:28Z

Ohhhhh, I see what you’re getting at. The equivalent of Office in our use case would be NapariSettings. I didn’t consider adding something to encode ShortcutSettings as a way to properly encode KeyBinding.

When I have time I’ll try adding a _json_encode method to ShortcutSettings and see if that fixes things :)

tlambert03 · 2022-10-01T15:49:47Z

yeah, give it a shot. I do still agree that it's frustrating that you can't have the logic that declares how a field should be json encoded on the field itself... rather than needing to take special action in the higher level models that use the field.

Note that napari/napari#2297 was trying to solve this internally for napari (e.g. if you define a _json_encode) method on the field type itself, napari's EventedModel will find it and use it. ... not 100% sure if it will work deeply nested here, but this is definitely a case where that was "supposed" to help. So you might also try that

kne42 · 2022-10-04T16:55:58Z

I made a branch experimenting with the above, but ran into an issue with representing fully-default models. Tried to debug but just more and more issues keep popping up due to our custom handling of YAML among other things. I think at this point it would be simpler to proceed with this PR with some of your requested changes, so that's what I'll do.

liu-ziyang · 2022-10-04T20:48:27Z

If I understand correctly this would be superseded by #68?

kne42 · 2022-10-04T20:59:33Z

yes

tlambert03 · 2022-10-10T20:35:23Z

@kne42 can I close this now with #68 ?

tlambert03 · 2022-10-10T20:53:15Z

closing... but feel free to reopen if I'm mistaken!

kne42 · 2022-10-12T17:18:56Z

Re-opening this, ran into some more issues where we couldn't generate the schema from KeyBinding since it "cannot be serialized as JSON" again as the schema function does not actually take into account the encoders we provide (perhaps another pydantic bug?). That was my bad for not running additional tests.

Do you think another solution would be better? We could theoretically subclass KeyBinding from str but things would get quite messy/confusing. Also, the latest implementation with the custom root type errors when it cannot parse a valid KeyCombo whereas previous versions would still store the parts as Key.UNKNOWN, is this desirable functionality?

tlambert03 · 2022-10-17T12:01:25Z

just curious, how did using napari's _json_encode work out?

tlambert03 · 2022-10-17T12:03:40Z

more context about the issues you're running into would also be appreciated. minimal examples using pydantic are helpful, but i know that there are lots of little tricky bits with pydantic, so seeing the actual code and problems you're running into would be very helpful here so we're not just talking in the abstract

my gut feeling here is that using __root__ isn't necessarily any better or more appropriate here. For better or worse, Issues of serialization are often best dealt with on the models that you want to serialize, and not on the field itself.

kne42 · 2022-10-17T21:30:01Z

just curious, how did using napari's _json_encode work out?

It worked for .json(models_as_dict=False), however it didn't apply to .yaml() as that had a different set of encoding features and our implementation of it did not have an equivalent option for models_as_dict so I had to re-implement that, however it created a huge hard-to-maintain mess inside the napari codebase and was finicky since we were doing custom logic for removing empty keys.

more context about the issues you're running into would also be appreciated. minimal examples using pydantic are helpful, but i know that there are lots of little tricky bits with pydantic, so seeing the actual code and problems you're running into would be very helpful here so we're not just talking in the abstract

this is the failing test, basically even though we have our own json encoders defined, when it comes to generating the schema, they... just use their default one! :))))))) im at the point where i just want to rewrite a ton of pydantics source code

tlambert03 · 2022-10-17T21:35:09Z

when it comes to generating the schema, they... just use their default one

ohhh... I see, your issue is in schema_json()... did you try __modify_schema__?
https://pydantic-docs.helpmanual.io/usage/schema/#modifying-schema-in-custom-fields

im at the point where i just want to rewrite a ton of pydantics source code

well you're in luck, they're hard at work on v2, re-written in rust.

tlambert03 · 2022-10-17T21:40:21Z

as a side comment here: I also think that using the json-schema autogenerator thing in the napari preferences dialog has been stretched well past its original scope (and usefulness) ... so another thing you could consider here is simply excluding this from the schema there, and creating a custom widget without going through json-schema-form-thingy

kne42 · 2022-10-17T23:10:31Z

ohhh... I see, your issue is in schema_json()... did you try __modify_schema__? https://pydantic-docs.helpmanual.io/usage/schema/#modifying-schema-in-custom-fields

yes, turns out that's called later down the chain after it's already been passed to their default encoding function which throws an error

well you're in luck, they're hard at work on v2, re-written in rust.

thank god im actually at my wit's end with this library's design, perhaps it's time to learn rust and contribute 😓

tlambert03 · 2022-10-17T23:45:07Z

yes, no doubt there are frustrations. but i think on the whole, it's an excellent idea and library, with some hiccups in the original implementation. (something I think we should all be able to relate to as napari developers 😂) each time I find myself thinking I could do better, I find it harder than at first I thought

feat: use custom root type for KeyBinding

a848d6a

kne42 requested review from tlambert03 and andy-sweet September 29, 2022 21:09

kne42 added 2 commits September 29, 2022 15:45

test: improve tests and erroring

8b2fc29

test: improve coverage

e524085

kne42 mentioned this pull request Sep 30, 2022

Use app-model keybindings internally napari/napari#5103

Merged

andy-sweet approved these changes Sep 30, 2022

View reviewed changes

tlambert03 reviewed Sep 30, 2022

View reviewed changes

tlambert03 closed this Oct 10, 2022

kne42 deleted the keybind-custom-root-type branch October 11, 2022 17:09

kne42 restored the keybind-custom-root-type branch October 12, 2022 00:41

kne42 mentioned this pull request Oct 12, 2022

feat: use custom root type for KeyBinding #70

Closed

kne42 mentioned this pull request Oct 17, 2022

represent keybind as string #73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use custom root type for KeyBinding #65

feat: use custom root type for KeyBinding #65

kne42 commented Sep 29, 2022

codecov bot commented Sep 29, 2022 •

edited

andy-sweet left a comment •

edited

andy-sweet Sep 30, 2022

andy-sweet Sep 30, 2022

andy-sweet Sep 30, 2022

tlambert03 commented Sep 30, 2022 •

edited

tlambert03 Sep 30, 2022

tlambert03 commented Sep 30, 2022 •

edited

kne42 commented Sep 30, 2022

tlambert03 commented Sep 30, 2022 •

edited

kne42 commented Oct 1, 2022

tlambert03 commented Oct 1, 2022

kne42 commented Oct 4, 2022

liu-ziyang commented Oct 4, 2022

kne42 commented Oct 4, 2022

tlambert03 commented Oct 10, 2022

tlambert03 commented Oct 10, 2022

kne42 commented Oct 12, 2022

tlambert03 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022 •

edited

kne42 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022

kne42 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022

		@@ -100,6 +117,37 @@ def test_in_dict():
		assert kbs[hash(new_a)] == 0


		def test_errors():

feat: use custom root type for KeyBinding #65

feat: use custom root type for KeyBinding #65

Conversation

kne42 commented Sep 29, 2022

codecov bot commented Sep 29, 2022 • edited

Codecov Report

andy-sweet left a comment • edited

Choose a reason for hiding this comment

andy-sweet Sep 30, 2022

Choose a reason for hiding this comment

andy-sweet Sep 30, 2022

Choose a reason for hiding this comment

andy-sweet Sep 30, 2022

Choose a reason for hiding this comment

tlambert03 commented Sep 30, 2022 • edited

tlambert03 Sep 30, 2022

Choose a reason for hiding this comment

tlambert03 commented Sep 30, 2022 • edited

kne42 commented Sep 30, 2022

tlambert03 commented Sep 30, 2022 • edited

kne42 commented Oct 1, 2022

tlambert03 commented Oct 1, 2022

kne42 commented Oct 4, 2022

liu-ziyang commented Oct 4, 2022

kne42 commented Oct 4, 2022

tlambert03 commented Oct 10, 2022

tlambert03 commented Oct 10, 2022

kne42 commented Oct 12, 2022

tlambert03 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022 • edited

kne42 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022

kne42 commented Oct 17, 2022

tlambert03 commented Oct 17, 2022

codecov bot commented Sep 29, 2022 •

edited

andy-sweet left a comment •

edited

tlambert03 commented Sep 30, 2022 •

edited

tlambert03 commented Sep 30, 2022 •

edited

tlambert03 commented Sep 30, 2022 •

edited

tlambert03 commented Oct 17, 2022 •

edited