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

Disable DynamicMap._deep_indexable in display hook exceptions #848

Merged
merged 2 commits into from
Sep 4, 2016

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Sep 4, 2016

I am not sure this is the correct fix (setting DynamicMap as not being ``_deep_indexable) but it is a simple fix for the issue with style restoration on DynamicMaps in the display hooks.

The issue was that StopIteration was being raised in traverse on dynamic maps. There are two other possible approaches I can think of:

  1. Give DynamicMap an alternative implementation of traverse. I don't like this much as we rely on traverse a lot and expect it to always behave in a consistent way.
  2. Temporarily declare DynamicMap as not being deep_indexable during style restoration.

I might look into option 2. now as it would be a less dramatic change.

@jlstevens
Copy link
Contributor Author

jlstevens commented Sep 4, 2016

The following diff also fixes the issue. I'm not sure I want to commit it as it is fairly ugly and maybe we do want to disable deep indexing for DynamicMaps?

         global FULL_TRACEBACK
         if Store.current_backend is None:
             return
+        DynamicMap._deep_indexable = False
         optstate = StoreOptions.state(element)
+        DynamicMap._deep_indexable = True
         try:
             html = fn(element,
                       max_frames=OutputMagic.options['max_frames'],
@@ -115,7 +117,10 @@ def display_hook(fn):
             sys.stderr.write("Rendering process skipped: %s" % str(e))
             return None
         except AbbreviatedException as e:
-            try:    StoreOptions.state(element, state=optstate)
+            try:
+                DynamicMap._deep_indexable = False
+                StoreOptions.state(element, state=optstate)
+                DynamicMap._deep_indexable = True
             except: pass
             FULL_TRACEBACK = '\n'.join(traceback.format_exception(e.etype,
                                                                   e.value,
@@ -126,7 +131,10 @@ def display_hook(fn):
             return "<b>{name}</b>{msg}<br>{message}".format(msg=msg, **info)

         except Exception as e:
-            try:    StoreOptions.state(element, state=optstate)
+            try:
+                DynamicMap._deep_indexable = False
+                StoreOptions.state(element, state=optstate)
+                DynamicMap._deep_indexable = True
             except: pass
             raise
     return wrapped

Edit 1: I should add that I can't easily put this enabling/disabling in StoreOptions.state itself due to cyclic import issues. In other words, I don't believe I can import DynamicMap into core.options.

Edit 2: I suppose I could just wrap this into a function in the display hooks. It would be tidier so I might go and commit that now.

@jlstevens jlstevens force-pushed the dynamic_style_restoration_skip branch from d8782b8 to 4f89b18 Compare September 4, 2016 17:49
@jlstevens jlstevens force-pushed the dynamic_style_restoration_skip branch from 4f89b18 to b179622 Compare September 4, 2016 17:51
@jlstevens
Copy link
Contributor Author

I have now gone for the latter approach, setting _deep_indexable to False temporarily in the display hooks.

@philippjfr
Copy link
Member

I have now gone for the latter approach, setting _deep_indexable to False temporarily in the display hooks.

That sounds okay although it is not the correct fix. The problem is that the options state that was initially recorded for the displayed object is no longer long enough because the DynamicMap has potentially created more items. The only way I can see of doing this correctly is to collect both the id and the objects in the initial traverse and then additionally collect all the objects that were styled dynamically. That way you would collect all the objects and their original ids, which you can iterate over to restore the original ids.

@jlstevens jlstevens changed the title Declared DynamicMap as not being _deep_indexable Disable DynamicMap._deep_indexable in display hook exceptions Sep 4, 2016
@philippjfr
Copy link
Member

If you want to leave it at this fix for now that's okay, but we should keep an issue open to make sure we implement the proper fix at some point whatever form that will take.

@jlstevens
Copy link
Contributor Author

The problem is that the options state that was initially recorded for the displayed object is no longer long enough because the DynamicMap has potentially created more items.

Yes, that's right. optstate = option_state(element) runs before html = fn(element, ...) which in the case of DynamicMap creates new elements.

I agree this is a temporary fix and there is already an issue to track this #813. In future there will be a PR to fix this properly - I think it will be tricky as DynamicMaps already have their own approach to styling elements.

@jlstevens
Copy link
Contributor Author

If this quick fix isn't too ugly, I think it is ready to merge. I've assigned the corresponding issue to the 1.7 milestone to make sure we don't forget to get a better fix in place before then.

@philippjfr
Copy link
Member

As a temporary fix this is fine, since it's currently broken in a number of ways and is actually likely to restore the wrong id to various objects. Merging.

@philippjfr philippjfr merged commit d6b41b2 into master Sep 4, 2016
@philippjfr philippjfr deleted the dynamic_style_restoration_skip branch September 27, 2016 22:22
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants