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

Magic preprocessors for conversion to vanilla Python #1491

Merged
merged 54 commits into from Jun 4, 2017

Conversation

Projects
None yet
3 participants
@jlstevens
Member

jlstevens commented May 28, 2017

This is very much a work-in-progress prototype. I will update this description if we decide to go ahead with this approach and after I have pushed forced a cleaner commit history.

@jlstevens jlstevens added the WIP label May 28, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 29, 2017

@jbednar @philippjfr Tests are passing so we can now discuss the main change - the refactor of the output magic to allow a new hv.util.output utility to work the same way in pure Python.

The refactor moves almost all the logic to an OutputOptions class so the actual magic class is tiny. An instance of this class is then kept on the Store so it can be used by the output magic and the utility.

I think this is the right thing to do. My major misgiving here is this is not code we like (or are at all proud of!) and I am worried to see this horrible code escape the ipython module where it was safely contained until now.

OutputMagic.allowed['fig'] = list_formats('fig', backend)
OutputMagic.allowed['holomap'] = list_formats('holomap', backend)
Store.output_options = OutputOptions()

This comment has been minimized.

@philippjfr

philippjfr May 29, 2017

Member

Don't quite get this, Store.output_options is an instance but all the actual configuration is set on the class? Can it all be class based?

This comment has been minimized.

@jlstevens

jlstevens May 29, 2017

Member

Maybe although there is this:

    def __init__(self, *args, **kwargs):
        self.shell = kwargs.pop('shell', None)
        super(OutputOptions, self).__init__(*args, **kwargs)
        self.output.__func__.__doc__ = self._generate_docstring()

I can try setting shell as a class attribute and removing this, though it will lose the generated docstring.

This comment has been minimized.

@philippjfr

philippjfr May 29, 2017

Member

When would a user every look at the OutputOptions? Seems like internal API to me.

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 29, 2017

Switching everything to classmethods seems to have worked, though it does mean the param generated docstring on the magic has been lost.

@jbednar

This comment has been minimized.

Member

jbednar commented May 30, 2017

Sounds great! Are you using the PNG export option from the latest Bokeh, or some other mechanism for Bokeh?

@philippjfr

This comment has been minimized.

Member

philippjfr commented May 30, 2017

Are you using the PNG export option from the latest Bokeh?

Yes, see #1493. You can use it in the notebook too but it's slow so not too useful for interactive use.

@jbednar

This comment has been minimized.

Member

jbednar commented May 30, 2017

Ok, great! Looks like this support arrived just in time for what we needed it for!

def __call__(self, options=None, **kwargs):
self.warning('Use opts method instead')

This comment has been minimized.

@philippjfr

philippjfr Jun 3, 2017

Member

I'm wary about merging this right straight away. This will overwhelm users with warnings and a lot of our docs and examples are still using __call__. We need to come up with a very soft deprecation cycle for this. I'd even say that we should have one release where the only thing nudging users towards using .opts is consistent usage of .opts throughout our docs. Anything else is just too disruptive and annoying imo. At the very minimum all docs/example should be updated before warning. @jbednar thoughts?

This comment has been minimized.

@jlstevens

jlstevens Jun 3, 2017

Member

This was mostly an experiment and having actually seen the results, I agree.

I'd even say that we should have one release where the only thing nudging users towards using .opts is consistent usage of .opts throughout our docs.

That is a start though can we add this warning for the release cycle following that? I'll remove this warning from this PR.

This comment has been minimized.

@jlstevens

jlstevens Jun 3, 2017

Member

We can do the following things:

  1. Document using opts only (no argument there!)
  2. Mention that people should please stop using __call__ in the release notes where we mention the addition of opts.
  3. When we do enable the warning, you should be able to disable it via the config system we really want and need (set deprecation settings, set styles, other things like that!)
@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 3, 2017

Note: We need to allow the opts string syntax (i.e using the parser) everywhere. Including where options can be set for saving/outputting data from the renderers.

@jbednar

This comment has been minimized.

Member

jbednar commented Jun 3, 2017

Do we have a way that the user could explicitly enable a warning? If so we could suggest that they do so in the release notes, but otherwise not warn for now.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 3, 2017

Do we have a way that the user could explicitly enable a warning?

Right that is another alternative. This is what I mean about wanting a 'config' system - to set things like this.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 4, 2017

@philippjfr I'm expecting the tests to go green and I think this PR should now be reviewed/merged.

It isn't 100% polished or complete but it is probably best to merge sooner rather than later, to help with debugging and to allows us to continue with the docs.

Outstanding work

Here are the things introduced in this PR that need finishing off/fixing. These can be associated with issues as necessary:

  • examples/README.rst and examples/examples.py are incomplete. I intend for examples.py to generate an index and allow grepping of notebooks referred to in the user guide.
  • There is a FIXME in one of the notebook preprocessor tests relating to multiple expressions in the line line of a cell.
  • The parsing of the opts string in the opts method is very rough right now and needs to be made more robust.
  • The holoviews command/entry point is also quite rough and needs to be improved e.g decent command line help using argparse etc.
  • Using hv.util.opts and hv.util.output in the docs - as our docs are being rewritten, we must make sure to use these now!
  • Improve validation in hv.util.opts to match the magic.

Edit: Related ideas

  • I think the idea of hv.config and a load_backend utility (used by the notebook_extension) should be added to the new utils subpackage in separate PRs.
  • In theory we could have an additional (optional) preprocessor to wrap expressions at the ends of all cells is something like show (e.g pop up a window for matplotlib). With this output side-effect the hv.util.output might make sense when replacing a cell magic...

Although there are a few loose bits and pieces, the bulk of what this PR offers is the hv.util.opts and hv.util.output helpers that can be used from pure Python instead of the magics. E.g hv.util.output(fig='svg') will work as will the string version hv.util.output("fig='svg'"). The opts utility only accepts the string specifications used by the magic.

In addition, this PR adds a collection of preprocessors that make use of these utils to convert notebooks to simple .py scripts. One of the main things this will enable is launching notebooks as bokeh apps with minimal (if any!) changes.

@@ -12,7 +12,7 @@
commit="$Format:%h$", reponame='holoviews')
from .core import archive # noqa (API import)
from .core.dimension import OrderedDict, Dimension # noqa (API import)
from .core.dimension import OrderedDict, Dimension, Dimensioned # noqa (API import)

This comment has been minimized.

@philippjfr

philippjfr Jun 4, 2017

Member

How come we need Dimensioned in the top-level namespace?

This comment has been minimized.

@jlstevens

jlstevens Jun 4, 2017

Member

Thanks! That is a stray/unnecessary import now - I still need Dimensioned in util.__init__.py though...

This comment has been minimized.

@jlstevens
except:
options = OptsSpec.parse(
'{clsname} {options}'.format(clsname=self.__class__.__name__,
options=options))

This comment has been minimized.

@philippjfr

philippjfr Jun 4, 2017

Member

Seems okay for now but it might be better to list an explicit exception.

This comment has been minimized.

@jlstevens

jlstevens Jun 4, 2017

Member

Sure. This falls under my bullet points for something to improve. The exception is part of it, but the whole try/except here isn't a very robust approach. I can specify an exception now but this whole thing needs improving (in another PR imho).

This comment has been minimized.

@jlstevens

jlstevens Jun 4, 2017

Member

Specified SyntaxError in a325041

Cell magic version acts as a no-op.
"""
if obj is not None:
return obj

This comment has been minimized.

@philippjfr

philippjfr Jun 4, 2017

Member

Did we want to allow this in a later PR? I remember discussing that all it has to do is temporarily set the renderer options.

This comment has been minimized.

@jlstevens

jlstevens Jun 4, 2017

Member

Could you outline how you can imagine that working? Even if I set the options state temporarily for an expression, it will have no effect unless that expression has some sort of side-effect, namely output. The cell magic resets everything after display and this concept of 'display' doesn't exist outside the notebook.

If you do have ideas about this, I would like to hear them as I would like these utilities to correspond to the magics as closely as possible...

This comment has been minimized.

@philippjfr

philippjfr Jun 4, 2017

Member

Good point probably doesn't make sense afterall.

This comment has been minimized.

@jlstevens

jlstevens Jun 4, 2017

Member

I added a suggestion above regarding a potential show call that could be injected by an optional preprocessor. Something to think about (maybe it isn't worth it?) and definitely not for this PR.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 4, 2017

Looks good, as you laid out in your summary there's some more work to be done here but the PR looks to be in a mergeable state. Happy to merge when you tell me.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 4, 2017

@philippjfr Let's merge once the tests are green.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 4, 2017

Great, merging.

@philippjfr philippjfr merged commit 91a0917 into master Jun 4, 2017

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 remained the same at 78.931%
Details
s3-reference-data-cache Test data is cached.
Details
@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 4, 2017

I've added another item to the list above regarding validation. We need to make sure we address all the things in that list.

@philippjfr philippjfr deleted the opts_preprocessor_prototype branch Jun 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment