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

Correctly infer document from bokeh.io._state for bokeh.io.save (instead of creating new one) #5978

Merged
merged 13 commits into from Mar 15, 2017

Conversation

@canavandl
Copy link
Contributor

canavandl commented Mar 10, 2017

  • issues: fixes #5977
  • tests added / passed
  • release document entry (if new feature or API change)

Refactor _save_helper to use internally use _ModelInDocument (via bokeh.embed.file_html) in order to correctly handle document state.

For testing purposed, you can use the bug example from the issue (#5977)

Migration Note:

  • bokeh.io.save now will only accept a LayoutDOM item and no longer a document. I don't think this should be much of an interruption, as I don't think any users are saving documents. This also brings the save method in line with the bokeh.io.show method, making things more intuitive.
  • validate kwarg to bokeh.io.save has been deprecated
  • bokeh.embed.standalone_html_page_for_models has been marked as deprecated in place of bokeh.embed.file_html
filename (str, optional) : filename to save document under (default: None)
If None, use the default state configuration, otherwise raise a
``RuntimeError``.

This comment has been minimized.

@canavandl

canavandl Mar 10, 2017 Author Contributor

This docstring is out-of-date. The implementation now will create a tempfile matching the __file__ name instead of raising a RuntimeError.

This comment has been minimized.

@bryevdv

bryevdv Mar 10, 2017 Member

Please update any docstrings that need updating :)

This comment has been minimized.

@canavandl

canavandl Mar 10, 2017 Author Contributor

done

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

migration note for obj param type change and validate deprecation / ignore

This comment has been minimized.

@canavandl

canavandl Mar 14, 2017 Author Contributor

added

bokeh/io.py Outdated
raise RuntimeError("Unable to save object of type '%s'" % type(obj))
doc = state.document

if obj not in doc.roots:

This comment has been minimized.

@bryevdv

bryevdv Mar 10, 2017 Member

I think checking obj.document is None is better? That handles the case where the obj is in a doc whether the doc is the state.document "curdoc" or some different explicitly created document. And if obj.document is None then the obj is not in doc.roots so good to add as a fallback

@@ -105,11 +105,6 @@ def test_default_filename(self):
filename, resources, title = io._get_save_args(io._state, None, "resources", "title")
self.assertEqual(filename, "filename")

def test_missing_filename(self):

This comment has been minimized.

@canavandl

canavandl Mar 10, 2017 Author Contributor

This test didn't actually test for a missing filename (which now is handled without raising an exception and tested elsewhere). It just happened to raise a RuntimeError related to trying to save an object of type(str).

bokeh/io.py Outdated
remove_after = False
if isinstance(obj, LayoutDOM):
if obj.document is None:
Document().add_root(obj)

This comment has been minimized.

@canavandl

canavandl Mar 10, 2017 Author Contributor

This is cause of the noted bug (themes not being applied). The solution is the get the document from io._state and not create a new one.

This comment has been minimized.

@bryevdv

bryevdv Mar 10, 2017 Member

See what was done for themes in https://github.com/bokeh/bokeh/pull/5932/files I think we have to do the same thing. Is there a reason save can't just be implemented in terms of components ?

This comment has been minimized.

@canavandl

canavandl Mar 10, 2017 Author Contributor

It's currently implemented via file_html which uses the new _ModelInDocument code to handle theming. I'm not sure what you'd hope to gain from rewriting.

This comment has been minimized.

@bryevdv

bryevdv Mar 10, 2017 Member

If that's the case why do we need to do anything with docs or roots right here? I'm trying not to duplicate what _ModelInDocument is doing, and it seems like we are at least partially doing that below.

@canavandl canavandl changed the title Correctly infers document from bokeh.io._state for bokeh.io.save (instead of creating new one) Correctly infer document from bokeh.io._state for bokeh.io.save (instead of creating new one) Mar 10, 2017
bokeh/io.py Outdated

if obj.document is None:
doc.add_root(obj)
remove_after = True

if validate:
doc.validate()

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

@canavandl just checking, decisions was to remove all the above code and just call standalone_html_page_for_models with obj, directly at the start of _save_helpers, since it uses _ModelInDocument internally?

This comment has been minimized.

@canavandl

canavandl Mar 13, 2017 Author Contributor

done

'''
if state is None:
state = _state

filename, resources, title = _get_save_args(state, filename, resources, title)
_save_helper(obj, filename, resources, title, validate)
_save_helper(obj, filename, resources, title)

This comment has been minimized.

@canavandl

canavandl Mar 13, 2017 Author Contributor

need to warn that the validate arg has been no-op'ed. Now it always happens inside of _ModelInDocument

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

This is a private function. Just get rid of it, if it's not used anymore.

This comment has been minimized.

@canavandl

canavandl Mar 13, 2017 Author Contributor

I commented that poorly, the validate value comes from the bokeh.io.save function signature, which is user facing. How do we deprecate kwargs in Bokeh?

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

If they are on public APIs by checking for them and calling deprecated explicitly, and then doing whatever fix up is necessary to not propagate the parameter further inside the function.

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

Alternatively we can keep it and pass validate through to _ModelInDoc I guess it comes down to whether we want to allow for skipping validation. Though if the old save went through _ModelInDoc I guess we've already effectively been ignoring whether to validate.

This comment has been minimized.

@canavandl

canavandl Mar 13, 2017 Author Contributor

We were previously validating an empty document, so essentially not doing validation. I've just removed/deprecated the validate kwarg.

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

Fair enough. FYI CI will fail until scripy docs site is back up.

@canavandl
Copy link
Contributor Author

canavandl commented Mar 13, 2017

I believe this is ready for final review. ping @bryevdv

filename (str, optional) : filename to save document under (default: None)
If None, use the default state configuration, otherwise raise a
``RuntimeError``.

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

migration note for obj param type change and validate deprecation / ignore

@@ -659,6 +658,7 @@ def standalone_html_page_for_models(models, resources, title):
UTF-8 encoded HTML
'''
deprecated((0, 12, 5), 'bokeh.io.standalone_html_page_for_models', 'bokeh.io.file_html')

This comment has been minimized.

@bryevdv

bryevdv Mar 13, 2017 Member

migration note for function deprecation

This comment has been minimized.

@canavandl

canavandl Mar 14, 2017 Author Contributor

added

@bryevdv
Copy link
Member

bryevdv commented Mar 13, 2017

LGTM I think it just needs a migration note in 0.12.5.rst

(moved MIGRATION to the issue, I believe that is the current agreed place)

@bryevdv bryevdv removed the MIGRATION label Mar 13, 2017
the document before outputting a file.

The ``bokeh.io.save`` method will now only accept a ``LayoutDOM``object and no
longer a ``Document`` for its ``obj`` argument. This aligns the

This comment has been minimized.

@bryevdv

bryevdv Mar 14, 2017 Member

missing space before "object"

This comment has been minimized.

@bryevdv

bryevdv Mar 14, 2017 Member

also pure preference but I usually just say "has been deprecated"

@bryevdv
Copy link
Member

bryevdv commented Mar 14, 2017

@canavandl there is a conflict to resolve?

@bryevdv
Copy link
Member

bryevdv commented Mar 15, 2017

Failing only due to known issue with scipy.org being down. Merging now.

@bryevdv bryevdv merged commit 5a0aff2 into master Mar 15, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@bryevdv bryevdv deleted the canavandl/save_document branch Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.