Skip to content

Notebook command line parameters not checked #1639

Open
matthew-brett opened this Issue Apr 20, 2012 · 5 comments

3 participants

@matthew-brett

I just noticed that these command lines:

$ ipython notebook unlikely
$ ipython notebook unlikely=impossible
$ ipython notebook --pylab=impossible

all pass without remark, but this:

$ ipython notebook --unlikely=impossible

gives:

WARNING: Unrecognized alias: 'unlikely', it will probably have no effect.
[NotebookApp] Using existing profile dir: u'/Users/mb312/.ipython/profile_default'

Fernando commented on the list (in answer to my question whether this behavior is intended):

Yes, I think so, though I'm not sure how fixable it is, Min will have
a better idea: the problem is that the command-line handling captures
flags and then has to pass that information 'downstream' to all
components that make up an application, but it doesn't know in advance
what all the valid flags are from 'below'. But definitely file it so
we have a focused place to document/discuss this...

@minrk
IPython member
minrk commented Apr 20, 2012

These two:

$ ipython notebook unlikely
$ ipython notebook unlikely=impossible

are the same to IPython, and easy to catch. Terminal IPython takes positional args like this as files to run, which doesn't make sense in the notebook where these should raise.

The relevant state being:

# after parsing command-line
if self.extra_args:
    # we have positional args, but can't handle them
    raise ValueError("unhandled arguments: %s" % self.extra_args)

The difference between these two:

$ ipython notebook --pylab=impossible
$ ipython notebook --unlikely=impossible

is that the notebook knows what options are available to the subprocess, but does not know what are appropriate values.

This could be easy to do for pylab in particular, but probably not feasible for options in general, because argument validation is handled by traitlets when instantiating/configuring objects. The objects in the kernel subprocess are never instantiated in the Notebook process, so this mechanism is unavailable at startup.

It is a general disadvantage of our configuration system that the only real way to validate config is to construct all of the objects it is meant to configure, which makes it difficult to validate config for objects you are not creating immediately (or ever, as in this case).

@fperez
IPython member
fperez commented Apr 20, 2012

Mmh, an idea, Min: could we write a utility that is capable of validating a configurable without having to fully instantiate it? Here's what I'm thinking of:

class Foo(Configurable):
  def __init__(self, config, ...):
    # complicated constructor we can't easily match.

class FooMock(Foo):
  def __init__(self, config):
    pass # mock constructor
    # only do validation of config against own class-declared values

mock = FooMock(config)

If that last line doesn't raise any exception, we're good to go. The only problem I see is doing it recursively (since a configurable can hold other configurables), but we may be able to validate that too.

Does this sound feasible to you?

@minrk
IPython member
minrk commented Apr 20, 2012

Does this sound feasible to you?

I'm not sure. I do not think it is reasonable for us to maintain Mock versions of every Configurable in IPython, or to ensure all of the _trait_changed() methods have no undesirable side effects (importing Qt, pylab, etc.), so if any traits should ever do this, then I think it's a non-starter.

Without following the complete initialization path this has limited value (for instance Apps typically specify many args to their descendants), but it will do simple checking of enums like pylab.

I sketched up a version of this, and it does catch --pylab wrong. My main concerns are:

  • Triggering unnecessary side effects from _trait_changed methods
  • Doing something like this, where you are faking the initialization path, also dramatically increases the likelihood of catching errors that do not exist (especially if there is real code in __init__).
@fperez
IPython member
fperez commented Apr 20, 2012

I see, you're actually calling the real constructor there, so the risk of trouble from side effects is indeed very high. I was rather thinking of something much more static, hoping we could do only as much validation as makes sense from the traits themselves. But you're right, traits event handlers are really difficult to constrain behavior-wise. Mmh, I don't see an easy solution here...

@minrk
IPython member
minrk commented Apr 21, 2012

A side note:

The reason these args do not raise when you start the notebook is that they are not used by the notebook, other than to construct the default kernel args. In the long run, kernel args should be settable in the Notebook UI, so they can be different for your various kernels. Once this UI exists, that is where such output really belongs. The flow would be:

  • start kernel
  • see that kernel failed with bad config
  • tweak kernel cl-args, and try again
  • rinse, repeat

I think in the long run, it should probably be abnormal to specify kernel args at the ipython notebook call, and when we have good enough UI for kernel args (at least better than none at all, as right now), then perhaps we should drop support for this degenerate simultaneous specification of command-line args for two separate programs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.