Add potential fix for increasing map size bug #3699
Conversation
|
|
|
|
||
| if kwargs.get("state") is None: | ||
| state = dependency_state(views, drop_defaults=kwargs.get("drop_defaults")) | ||
| else: | ||
| state = kwargs["state"] | ||
|
|
||
| snippet = embed_snippet( | ||
| views, | ||
| drop_defaults=kwargs.get("drop_defaults", True), | ||
| state=state, | ||
| indent=kwargs.get("indent", 2), | ||
| embed_url=kwargs.get("embed_url", None), | ||
| requirejs=kwargs.get("requirejs", True), | ||
| cors=kwargs.get("cors", True) | ||
| ) |
There was a problem hiding this comment.
I think it would be good to explicitly add the arguments (just copy-paste them from embed_snippet).
Would also be good to update the docstring, which is not incorrect (the behaviour of state=None is different).
Maybe this should be an opt-in for backward compatibility. (state_tree_shake=False maybe?)
There was a problem hiding this comment.
If the correct behaviour is dependency_state and not get_manager_state, then I think the backwards compatibility relies on a bug 🤔 What do you think about passing a string instead, e.g. state="all" to include everything instead of having an argument toggle that switches between dependencies and full state (and only if state is None)
There was a problem hiding this comment.
I updated the documentation and also used a string to toggle between the modes:
state is None or equals "dependent": function will use dependency_state:
Current: 0.html: 0.0067501068115234375 MB
Current: 1.html: 0.0067501068115234375 MB
Current: 2.html: 0.0067501068115234375 MB
Current: 3.html: 0.0067501068115234375 MB
Current: 4.html: 0.0067501068115234375 MB
...
state equals "complete": function will use get_manager_state:
Current: 0.html: 0.006749153137207031 MB
Current: 1.html: 0.012719154357910156 MB
Current: 2.html: 0.01868915557861328 MB
Current: 3.html: 0.024659156799316406 MB
Current: 4.html: 0.03062915802001953 MB
...
state is a dictionary: function will use passed state direclty
Current: 0.html: 0.0007772445678710938 MB
Current: 1.html: 0.0007772445678710938 MB
Current: 2.html: 0.0007772445678710938 MB
Current: 3.html: 0.0007772445678710938 MB
Current: 4.html: 0.0007772445678710938 MB
...
Could you take another look on the code?
| if state is None or state == "dependent": | ||
| state = dependency_state(views, drop_defaults=drop_defaults) | ||
| elif state == "complete": | ||
| state = Widget.get_manager_state(drop_defaults=drop_defaults, widgets=None)['state'] |
There was a problem hiding this comment.
Would it make sense to move this logic into embed_data instead? Then we get support for these enums in more functions (and we also get to reuse the docstrings 😄).
| """ | ||
| snippet = embed_snippet(views, **kwargs) | ||
|
|
||
| if state is None or state == "dependent": |
There was a problem hiding this comment.
Changing the default (None case) is the most controversial here, as it is strictly speaking backwards incompatible. There might be a case for calling it a workaround for the poor GC of ipywidgets, but making this change should at least have some more eyes on it before merging.
There was a problem hiding this comment.
I understand your argument and it's difficult for me to estimate how much of an impact this change would have to other projects. Coming from ipyleaflet, it just looks like a bug to me and then I'd argue, that backwards compatibility should not rely on bugs. However, if other people really need the whole state and really have that implementation assumption as well, then I agree, we should stay backwards compatible as well. What do you think?
Your solution definelty will work in the end but as a user, it's not so clear to me when I'd use the dependent state and when the complete.
There was a problem hiding this comment.
I kind of agree it is a bug, we didn't implement this feature for nothing. I think this was the intended behavior, but yeah, fixing a bug can be considered a breaking change :)
There was a problem hiding this comment.
See original discussion here: #1387 (comment)
This PR fixes #3448 and jupyter-widgets/ipyleaflet#1098
Solution proposed by @maartenbreddels here: #3448 (comment)
I used binder and the code in the original issue to check if the file size still increases. It looks fine to me.