Command line flag do not take precedence over config.

[Carreau]

Cf ipython/ipython#9655 (comment)

Marking as critical as this was release in 4.1.

[fperez]

Yup, this is pretty bad... We need to add a test for this via a subprocess call to make sure it doesn't return.

[Carreau]

Yup, this is pretty bad... We need to add a test for this via a subprocess call to make sure it doesn't return.

There were quite a few regression in traitlets since 4.0, especially because the 4.x serie will impact LTS, I would really like to slow down on traitlets new feature and pay attention on quality and stability.

I think that things like #246, #242 , #230 are unnecessary and should be maybe reconsidered if we can't keep the core features stable.

[minrk]

Pretty sure I know roughly what I did to cause this. Assigning self to investigate.

[minrk]

It turns out the bug is in IPython (which does the CLI priority bit, not traitlets), but I think I can be defensive in traitlets and reintroduce the less-than-ideal behavior that IPython is implicitly relying upon.

IPython is relying on an improvement / bugfix in traitlets not being present (#42). The removal of deepcopy means that update_config actually updates config (correct behavior post-4.1), rather than removing and replacing it (problematic behavior in 4.0 and earlier, causing duplicate events to fire). IPython does this:

self.parse_command_line()
# save CLI config for later
cli_config = self.config
# implicitly assume that this replaces instead of modifying self.config
self.load_config_files()
# reapply cli_config to restore priority, assuming self.config is not the same object as self.config two lines ago
self.update_config(cli_config)

IPython relies on load_config_files reassinging self.config to a new Config object, which to me is relying on a bug, or at least unspecified behavior. A fix in IPython would be one line: create a copy of the CLI config to load later so it won't be modified, rather than assume copies are made implicitly. I'm planning three PRs:

1. a tiny fix for traitlets 4.2.2, restoring a minor deficiency in its behavior that IPython relied upon.
2. a tiny fix in IPython to not rely on traitlets bugs
3. a more substantial fix in traitlets that should make this harder to reintroduce

[minrk]

#249 is the 4.2.2 fix

[Carreau]

Awesome ! Thank for the in depth explanation !

On Fri, Jun 24, 2016 at 2:26 PM, Min RK notifications@github.com wrote:

#249 #249 is the 4.2.2 fix

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#248 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAUez82f5_Ecv8wxhWnJKT01vw-05S7Iks5qPEtxgaJpZM4I9Pwl
.

[minrk]

ipython/ipython#9660 is the IPython fix

[minrk]

And #250 is the traitlets (4.3) fix that should make it harder to reintroduce this bug in the future.

[fperez]

Thanks so much @minrk for this and the multiple PRs!

[fperez]

Sorry, I closed it thinking only of master/5.0, I guess it stays open as a backport.

[fperez]

Reopening it so ipython/ipython#9660 can be the real closer of the backport.

[Carreau]

Hum, ipython/ipython#9660 has been merged (and backported), Did you mean #250 ?

[fperez]

Ah, sorry! Then I'm confused: is this really targeting a 4.2.2 backport, or only 4.3? Because the main fixed was merged in master (hence for 5.0)...

[Carreau]

Ah, sorry! Then I'm confused: is this really targeting a 4.2.2 backport, or only 4.3? Because the main fixed was merged in master (hence for 5.0)...

The fix is #250, ipython/ipython#9660 is a "workaround".

[Carreau]

The fix is #250, ipython/ipython#9660 is a "workaround".

More precisely, ipython/ipython#9660 use traitlets API in the way it's supposed to be used,.
#250 reintroduce an old behavior of traitlets which is a implementation detail that should not be really relied upon, which IPython was using. So that's neither really an IPython bug, nor really a traitlets bug.

[minrk]

I created a 4.2.x branch from 4.2.1 with #249. I can release that now as 4.2.2, unless there's anything else we want in there.

Closing this one, as the backports on both traitlets and IPython have been made.

[fperez]

@Carreau, @takluyver, anything else you think is critical for 4.2.2?

[Carreau]

@Carreau, @takluyver, anything else you think is critical for 4.2.2?

Not from the top of my head, but I'm not following traitlets too much these days.

[takluyver]

Ditto

[fperez]

Green light then, @minrk!

[minrk]

Thanks, released.

[Carreau]

Appreciate that ! Thanks Min !

On Fri, Jul 1, 2016 at 2:20 AM, Min RK notifications@github.com wrote:

Thanks, released.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#248 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAUez5gzgpXHvtJs-2-iPHxV4JKfPsETks5qRNv6gaJpZM4I9Pwl
.