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

New BaseIPythonApplication #454

Closed
wants to merge 103 commits into from
Closed

New BaseIPythonApplication #454

wants to merge 103 commits into from

Conversation

ellisonbg
Copy link
Member

All IPython applications should subclass this new Application subclass.

os.path.join(get_ipython_package_dir(), u'config', u'profile')
)

config_file_paths = Tuple(Unicode, Unicode, Unicode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is config_file_paths a Tuple, and not a List? The resulting requirement that the length be 3 seems artificial.

@minrk
Copy link
Member

minrk commented May 18, 2011

The Application can load any config file by name. The BaseApplication disables this, by hardwiring loading the application default or profile. BaseIPythonApplication must support loading config files by explicit path as well (this is easily accomplished by having the current behavior be the default, but not discarding the file argument of Application.load_config_file)

@minrk
Copy link
Member

minrk commented May 20, 2011

config files should have a priority lower than command-line specified options, however often the command-line must be parsed to locate those files. That means that currently, one has to do something like the following to ensure proper priority:

app = MyApp()
app.parse_command_line()
cfg = app.config
app.load_config_file(app.located_cfg_file)
# restore cl priority
app.update_config(cfg)

@minrk
Copy link
Member

minrk commented May 20, 2011

Should we display the default values for configurables? It should be easy (in fact, I've already done it locally) to make the Configurable output:

Foo.i : Int [ default: 1 ]
    helpdocs...

@minrk
Copy link
Member

minrk commented May 27, 2011

Profiles are now directories, if anyone wants to review that code (ProfileDir adapted from ClusterDir, now in core.newapplication)

@minrk
Copy link
Member

minrk commented May 29, 2011

I'm working on updating the base IPython App to use the new base class, and I see that the wthread/qthread options are listed as deprectaed. Do we want to take this opportunity to remove them, as there will be already be very little command-line compatibility with the old code?

@minrk
Copy link
Member

minrk commented May 29, 2011

Another note: The current config has no avenue for positional arguments, so there's no way to do 'ipython foo.py'. Do we want to allow this?

@fperez
Copy link
Member

fperez commented May 29, 2011

On Sun, May 29, 2011 at 2:05 PM, minrk
reply@reply.github.com
wrote:

Another note: The current config has no avenue for positional arguments, so there's no way to do 'ipython foo.py'.  Do we want to allow this?

Yes, I think so. It will be seen as a major regression and annoyance
if we don't, as it's a common way today for people to get a script
executed under IPython (better exception reporting, etc) when they
need to exit to the command line in order to do things like forcing a
reload of extension modules.

@minrk
Copy link
Member

minrk commented May 29, 2011

Okay, I think that's going to make a mess of things.

Currently, since everything is so explicit, it's unambiguous to determine what people want. However, if we want to allow arbitrary extra arguments, it's more difficult to determine if what they did was invalid.

For instance, in an app that has subcommands, we would have:
ipython notebook, and if you do something like ipython notebok, you currently get an 'invalid subcommand' error.
But if we allow ipython foo.py, then I don't see a way to have anything other than ipython notebok replying with anything other than file 'notebok' not found. Essentially, we can't check arguments in the loader anymore, since everything becomes a valid extra argument.

Is there a possibility that these args will ever be anything but files? If not, then that helps a lot.

@minrk
Copy link
Member

minrk commented May 29, 2011

I've implemented extra_flags in the Application/KVLoader - the Loader just saves every argument it doesn't recognize in extra_args, so it's up to the Application to decide what's valid.

@fperez
Copy link
Member

fperez commented May 29, 2011

On Sun, May 29, 2011 at 4:14 PM, minrk
reply@reply.github.com
wrote:

I've implemented extra_flags in the Application/KVLoader - the Loader just saves every argument it doesn't recognize in extra_args, so it's up to the Application to decide what's valid.

OK, that seems like the best solution. That's indeed where any
semantics should be implemented in terms of giving meaning to the
contents of extra_args. Thanks.

@minrk
Copy link
Member

minrk commented May 30, 2011

I've got terminal IPython working with the new App class:
#485

@minrk
Copy link
Member

minrk commented May 30, 2011

Another thing to consider: Do we want warnings on unknown config values? Currently such things are ignored silently, so there's no feedback when you do something like: ipython profiel=mineother than your profile not being selected.

We can do this at two levels:

  1. Check top-level only, so we validate Configurable classes, but not traits
  2. Check each trait name

(1.) would cover the vast majority of cases, because a misspelled alias will be loaded as a top-level value, but doing things like ipython InteractiveShell.autocal=1 would still be ignored unless we check traits as well.

A disadvantage of having these warnings would be if people are using the same config file for multiple apps (e.g. QtConsole). Then it's likely that they will have configuration that only affects one or the other, which would produce these warnings.

@minrk
Copy link
Member

minrk commented May 31, 2011

I'm working on rewriting the Qt code to use newapp. There is a huge overlap in arguments/configuration between the terminal app I've already done and launching a Kernel. Should I split off a generic InteractiveShellApp class for both to inherit from, or should I make the KernelApp in IPython.zmq inherit from the terminal IPythonApp?

@minrk
Copy link
Member

minrk commented Jun 1, 2011

I've got the Kernels started using Applications now, so it's really only the QtConsoleApp that remains.

@minrk
Copy link
Member

minrk commented Jun 1, 2011

QtConsoleApp is now working in my qtconfig branch. I need to do a bit of cleanup and get the inline matplotlib backend hooked up to the configuration before I do the PR, but it works, and configuration does propagate down to the Kernel.

@minrk
Copy link
Member

minrk commented Jun 4, 2011

Do we have a model for module-level configuration? I simply created a SingletonConfigurable object for this in the inline matplotlib backend, but we should formalize an approach for this sort of thing, because it is relevant to #96 as well.

@ellisonbg
Copy link
Member Author

I think having a module level SingletonConfigurable is a fine approach. We have choosen a very particular model of config that basically uses classes as the core entity that is configured. I think this is a good model and thus, it makes sense to use a top-level class to hold module level config stuff. The only question is whether or not we want to standardize the name of the singleton. Maybe use the name module_config for the instance?

@minrk
Copy link
Member

minrk commented Jun 6, 2011

I've just been using the instance() method to get it, and not bothering with a named object, but module_config is a fine name.

Should we standardize the configurable's name as well? Perhaps CamelFileNameConfig (I think that's what I used for the inlinebackend, but I'll check).

@ellisonbg
Copy link
Member Author

I think the CamelFileNameConfig is a good choice and standardizing
that name makes sense. Do you think that instance() is better than
something like module_config? Is definitely is a more standard name.

On Mon, Jun 6, 2011 at 12:47 PM, minrk
reply@reply.github.com
wrote:

I've just been using the instance() method to get it, and not bothering with a named object, but module_config is a fine name.

Should we standardize the configurable's name as well?  Perhaps CamelFileNameConfig (I think that's what I used for the inlinebackend, but I'll check).

Reply to this email directly or view it on GitHub:
#454 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member Author

One more question. How does the config object get passed to the
module config classes?

On Mon, Jun 6, 2011 at 1:23 PM, Brian Granger ellisonbg@gmail.com wrote:

I think the CamelFileNameConfig is a good choice and standardizing
that name makes sense.  Do you think that instance() is better than
something like module_config?  Is definitely is a more standard name.

On Mon, Jun 6, 2011 at 12:47 PM, minrk
reply@reply.github.com
wrote:

I've just been using the instance() method to get it, and not bothering with a named object, but module_config is a fine name.

Should we standardize the configurable's name as well?  Perhaps CamelFileNameConfig (I think that's what I used for the inlinebackend, but I'll check).

Reply to this email directly or view it on GitHub:
#454 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

minrk commented Jun 6, 2011

Since this sort of thing (tracking a single canonical instance that may or may not be initialized from anywhere) is exactly why an instance() method exists, I think it makes sense.

The inline backend is the only one so far, so I just passed the shell.config in the code where the backend is set up in pylabtools.

It would probably be nice to have an official way to get the current application's config object. Currently the following will work:

cfg = BaseIPythonApplication.instance().config

or similarly with InteractiveShell in place of BaseIPythonApplication for the current shell instance, though those will be the same except in cases where an InteractiveShell is being used outside an IPython app (or an IPApp without a Shell).

We could have a central Config object, possibly stored as IPython.config.current, that gets updated by the Applications and/or shells on configuration.

@fperez
Copy link
Member

fperez commented Jun 7, 2011

On Mon, Jun 6, 2011 at 2:13 PM, minrk
reply@reply.github.com
wrote:

We could have a central Config object, possibly stored as IPython.config.current, that gets updated by the Applications and/or shells on configuration.

I may be misunderstanding something, but it seems we should avoid
storing state in modules, that's the kind of global state that tends
to make life difficult in other areas later, no?

@ellisonbg
Copy link
Member Author

In general yes I think not storing state in modules makes sense. But,
there are some places we are doing it anyways (io, ioloop, etc.). But
I agree with Fernando that we don't want a module level global that is
the current config. I think the right way to store this is as the
config attribute of the Application singleton.

On Mon, Jun 6, 2011 at 8:07 PM, fperez
reply@reply.github.com
wrote:

On Mon, Jun 6, 2011 at 2:13 PM, minrk
reply@reply.github.com
wrote:

We could have a central Config object, possibly stored as IPython.config.current, that gets updated by the Applications and/or shells on configuration.

I may be misunderstanding something, but it seems we should avoid
storing state in modules, that's the kind of global state that tends
to make life difficult in other areas later, no?

Reply to this email directly or view it on GitHub:
#454 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

minrk commented Jun 7, 2011

@fperez Are you saying that we don't want a configurable for the inline backend? That's a module with module-level config. the wx inputhook would be the same. An alternative would be for these modules to query the shell instance for values, but I would argue against that (for the backend at least, less strongly for wx hook).

I am fine with not having a global config storage, but do we want a slightly cleaner way of getting the current active application than the parent class instance call above, or is that fine?

@ellisonbg
Copy link
Member Author

I think having a module level singleton configurable class is fine if
we access it using instance. That way the actual config attributes as
well as the configurable class are not stored as module level
attributes.

On Mon, Jun 6, 2011 at 9:51 PM, minrk
reply@reply.github.com
wrote:

@fperez Are you saying that we don't want a configurable for the inline backend? That's a module with module-level config.  the wx inputhook would be the same. An alternative would be for these modules to query the shell instance for values, but I would argue against that (for the backend at least, less strongly for wx hook).

I am fine with not having a global config storage, but do we want a slightly cleaner way of getting the current active application than the parent class instance call above, or is that fine?

Reply to this email directly or view it on GitHub:
#454 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

minrk commented Jun 7, 2011

Another thing for which we want access to the parent application is inheriting the logger. Do we want to rely on calling app = BaseIPythonApplication.instance() to get the logger object, or entrust the Application with calling obj = Object(config=self.config, log=self.log) for all objects that might want to log anything? In-between is giving the objects themselves references to the current app (obj = Object(app=self, config=config)).

@ellisonbg
Copy link
Member Author

I think we should access logging using BaseIPythonApplication.instance().

On Tue, Jun 7, 2011 at 2:55 PM, minrk
reply@reply.github.com
wrote:

Another thing for which we want access to the parent application is inheriting the logger.  Do we want to rely on calling app = BaseIPythonApplication.instance() to get the logger object, or entrust the Application with calling obj = Object(config=self.config, log=self.log) for all objects that might want to log anything? In-between is giving the objects themselves references to the current app (obj = Object(app=self, config=config)).

Reply to this email directly or view it on GitHub:
#454 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

minrk commented Jun 10, 2011

okay, then nothing needs to change.

Should I merge all of the separate App PRs into newapp now, so there's a single branch with everything up to date? There aren't any unresolved criticisms in any of those PRs at the moment.

Then I can post the merge of the Session objects as well, as that depends on merging parallel and qtapp.

minrk and others added 25 commits June 20, 2011 16:40
docstrings fleshed out, minor code cleanup.
also extract daemonize function from twisted.scripts._twistd_unix
the ipython-qtconsole script has been removed in favor of 'ipython qtconsole', but the ipython-qtconsole *GUI* script remains, when installed with setuptools.
* give ProfileDir its own file (core.profiledir)
* add core.profileapp for new subcommand

parallel docs updated to match
also fix typo introduced into ipcluster
* po/pi1/2 aliases have been removed
* default print of config help is on its own line
it is ignored if it is not first, causing SyntaxErrors
also make rekey accept basestring, not just str.

closes gh-532
message is now only printed once, and wrapped to 80 columns
Now, to instruct the profile to include parallel scripts, you would do:

$> ipython profile create foo --parallel
instead of
$> ipython profile create foo --cluster

docs updated accordingly.
_log_level_changed was defined more than once
* various classes have changed, profiles are dirs not files, etc.
* also add documentation of new command-line format
@minrk minrk closed this in fbd5ae9 Jun 21, 2011
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants