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

Projects
None yet
2 participants
@jlstevens
Copy link
Contributor

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

This comment has been minimized.

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 Sep 4, 2016

@jlstevens jlstevens force-pushed the dynamic_style_restoration_skip branch from 4f89b18 to b179622 Sep 4, 2016

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Sep 4, 2016

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

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Sep 4, 2016

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

This comment has been minimized.

Copy link
Contributor

philippjfr commented Sep 4, 2016

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

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Sep 4, 2016

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

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Sep 4, 2016

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

This comment has been minimized.

Copy link
Contributor

philippjfr commented Sep 4, 2016

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

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 69.338%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr removed the in progress label Sep 4, 2016

@philippjfr philippjfr deleted the dynamic_style_restoration_skip branch Sep 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.