don't preserve fixConsole output in json #1206

Merged
merged 3 commits into from Jan 27, 2012

Conversation

Projects
None yet
4 participants
@minrk
Member

minrk commented Dec 24, 2011

move utils.fixConsole calls closer to display, so they don't clobber the actual data stored in the notebook.

also store nonexistent input_prompt_number as null, rather than  , which is what should be drawn, not the actual value.

We need to be careful with this one, because currently we do write HTML-escaped text to the notebook (see #1205), so this will change the output. So if you open a notebook written by 0.12, plaintext output will be double-escaped until you run it again. The fixed output seems to display correctly in the old/bad code.

Let's not merge until we check if there's an easy way to avoid escaping to HTML a second time.

fixes #1205

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 24, 2011

Member

Just kidding - I wasn't diligent with my browser-refresh when switching branches. Bad notebooks (written by 0.12) loaded in this branch will see the HTML markup as plaintext, because the content will be double-escaped on load. Good notebooks (written by this branch) will be loaded unescaped ([0;31m in tact, output uncolored) in 0.12.

Of course, everything should be safe, so the only thing needed when moving from one to the other is to re-run the notebook to generate the outputs with/without the bug so it matches the running frontend.

I don't know if this is fixable, because the escaped HTML output is always perfectly valid as plaintext, so it's impossible to be certain about whether an extra escape should be made.

Note that this is not related to the notebook format, but a bug in the notebook frontend, which puts the wrong information in plaintext fields. However, one way to 'fix' the problem is to define this bug into the current notebook format, and increment the format with no changes. We then edit v2, and patch it to perform the escape/unescape around the files themselves. I do not think this is worth it, but it is an option.

Pinging @ellisonbg as notebook format coder-in-chief.

Member

minrk commented Dec 24, 2011

Just kidding - I wasn't diligent with my browser-refresh when switching branches. Bad notebooks (written by 0.12) loaded in this branch will see the HTML markup as plaintext, because the content will be double-escaped on load. Good notebooks (written by this branch) will be loaded unescaped ([0;31m in tact, output uncolored) in 0.12.

Of course, everything should be safe, so the only thing needed when moving from one to the other is to re-run the notebook to generate the outputs with/without the bug so it matches the running frontend.

I don't know if this is fixable, because the escaped HTML output is always perfectly valid as plaintext, so it's impossible to be certain about whether an extra escape should be made.

Note that this is not related to the notebook format, but a bug in the notebook frontend, which puts the wrong information in plaintext fields. However, one way to 'fix' the problem is to define this bug into the current notebook format, and increment the format with no changes. We then edit v2, and patch it to perform the escape/unescape around the files themselves. I do not think this is worth it, but it is an option.

Pinging @ellisonbg as notebook format coder-in-chief.

@hmeine

This comment has been minimized.

Show comment
Hide comment
@hmeine

hmeine Jan 6, 2012

Contributor

Would it be a possible third option to increment the nb format without adding the conversion code for now? Then, if someone decides that it is worth the extra code (and possibly volunteers to contribute it), one could already know which notebooks need which treatment.

Contributor

hmeine commented Jan 6, 2012

Would it be a possible third option to increment the nb format without adding the conversion code for now? Then, if someone decides that it is worth the extra code (and possibly volunteers to contribute it), one could already know which notebooks need which treatment.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 6, 2012

Member

incrementing the nbformat without adding the conversion code would mean that all your notebooks would immediately be considered unreadable, because they are the old format.

Member

minrk commented Jan 6, 2012

incrementing the nbformat without adding the conversion code would mean that all your notebooks would immediately be considered unreadable, because they are the old format.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 6, 2012

Member

I suppose that's imprecise - If we define the conversion as a no-op initially, then they would be readable and the old notebooks would be identifiable as you describe. New notebooks would be rejected as unreadable by IPython 0.12, and there's no way that updating the nbformat version would not cause that to happen.

The principal reason I am reluctant to fix this via the nbformat is that it is a frontend bug, and really has nothing to do with the notebook format, as evidenced by the fact that the fixes reside purely in javascript.

I would consider this the highest priority bug found in 0.12 so far, and the principal motivation for an 0.12.1 bugfix release, depending on which approach we chose.

Member

minrk commented Jan 6, 2012

I suppose that's imprecise - If we define the conversion as a no-op initially, then they would be readable and the old notebooks would be identifiable as you describe. New notebooks would be rejected as unreadable by IPython 0.12, and there's no way that updating the nbformat version would not cause that to happen.

The principal reason I am reluctant to fix this via the nbformat is that it is a frontend bug, and really has nothing to do with the notebook format, as evidenced by the fact that the fixes reside purely in javascript.

I would consider this the highest priority bug found in 0.12 so far, and the principal motivation for an 0.12.1 bugfix release, depending on which approach we chose.

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jan 6, 2012

Member

I agree with @minrk that this should be fixed in the javascript, not
the notebook format.

On Fri, Jan 6, 2012 at 1:54 AM, Min RK
reply@reply.github.com
wrote:

I suppose that's imprecise - If we define the conversion as a no-op initially, then they would be readable and the old notebooks would be identifiable as you describe.  New notebooks would be rejected as unreadable by IPython 0.12, and there's no way that updating the nbformat version would not cause that to happen.

The principal reason I am reluctant to fix this via the nbformat is that it is a frontend bug, and really has nothing to do with the notebook format, as evidenced by the fact that the fixes reside purely in javascript.

I would consider this the highest priority bug found in 0.12 so far, and the principal motivation for an 0.12.1 bugfix release, depending on which approach we chose.


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

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

Member

ellisonbg commented Jan 6, 2012

I agree with @minrk that this should be fixed in the javascript, not
the notebook format.

On Fri, Jan 6, 2012 at 1:54 AM, Min RK
reply@reply.github.com
wrote:

I suppose that's imprecise - If we define the conversion as a no-op initially, then they would be readable and the old notebooks would be identifiable as you describe.  New notebooks would be rejected as unreadable by IPython 0.12, and there's no way that updating the nbformat version would not cause that to happen.

The principal reason I am reluctant to fix this via the nbformat is that it is a frontend bug, and really has nothing to do with the notebook format, as evidenced by the fact that the fixes reside purely in javascript.

I would consider this the highest priority bug found in 0.12 so far, and the principal motivation for an 0.12.1 bugfix release, depending on which approach we chose.


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

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

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jan 15, 2012

Member

I will try to have a look at this one.

Member

ellisonbg commented Jan 15, 2012

I will try to have a look at this one.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jan 20, 2012

Member

I also agree that we shouldn't do a format number change unless it's really necessary, and in this case it isn't. Changing the nb format number has a pretty high cost, and we're starting to have a lot of users in the field, so we shouldn't make things hard for them without careful consideration of the benefit.

It's a bummer that this one is going to produce some glitches on load across this merge point, but there doesn't seem to be a way to avoid that...

Member

fperez commented Jan 20, 2012

I also agree that we shouldn't do a format number change unless it's really necessary, and in this case it isn't. Changing the nb format number has a pretty high cost, and we're starting to have a lot of users in the field, so we shouldn't make things hard for them without careful consideration of the benefit.

It's a bummer that this one is going to produce some glitches on load across this merge point, but there doesn't seem to be a way to avoid that...

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 25, 2012

Member

@ellisonbg @fperez any further thoughts on this one? I think it's the biggest bug in 0.12, and the fix has been outstanding for a over a month.

@fperez - there is a way to avoid the glitches - defining the bug into the notebook format, and incrementing the version, but I would put the costs of that (unreadability of 0.12.1 notebooks in 0.12) as much higher than the glitches themselves, which are trivially resolved by rerunning the notebook (works both directions).

I would vote for fixing the bug in the frontend (as done here), and pushing out 0.12.1 as a critical bugfix ASAP.

Member

minrk commented Jan 25, 2012

@ellisonbg @fperez any further thoughts on this one? I think it's the biggest bug in 0.12, and the fix has been outstanding for a over a month.

@fperez - there is a way to avoid the glitches - defining the bug into the notebook format, and incrementing the version, but I would put the costs of that (unreadability of 0.12.1 notebooks in 0.12) as much higher than the glitches themselves, which are trivially resolved by rerunning the notebook (works both directions).

I would vote for fixing the bug in the frontend (as done here), and pushing out 0.12.1 as a critical bugfix ASAP.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jan 26, 2012

Member

Should we apply this to master and cherry-pick it onto 0.12.1? I don't like cutting 0.12.1 in the middle of a bunch of other work that's being done, the notebook UI has changed significantly, etc. So I think our options are:

  • cherry pick just this onto 0.12 and call it 0.12.1. Obviously we'd also apply it to master.
  • keep going but try to stabilize very quickly so we can release 0.13 soon.

I actually think I'd prefer the latter. The notebook is so much nicer with the menus and the codemirror fixes we just merged, that I think it more than warrants being called 0.13. We could consider making a release in a 2-3 week timeframe, which would also be ideal for PyCon. Thoughts?

Member

fperez commented Jan 26, 2012

Should we apply this to master and cherry-pick it onto 0.12.1? I don't like cutting 0.12.1 in the middle of a bunch of other work that's being done, the notebook UI has changed significantly, etc. So I think our options are:

  • cherry pick just this onto 0.12 and call it 0.12.1. Obviously we'd also apply it to master.
  • keep going but try to stabilize very quickly so we can release 0.13 soon.

I actually think I'd prefer the latter. The notebook is so much nicer with the menus and the codemirror fixes we just merged, that I think it more than warrants being called 0.13. We could consider making a release in a 2-3 week timeframe, which would also be ideal for PyCon. Thoughts?

@hmeine

This comment has been minimized.

Show comment
Hide comment
@hmeine

hmeine Jan 26, 2012

Contributor

Great idea. I also think that the new notebook is much nicer (polished) than the 0.12 one, and that releasing it soon and as 0.13 would be warranted.

BTW: In case someone thinks that the polished appearance would be diminished by doubly-escaped HTML, how about detecting this and adding a notification (similar to modern browser’s "do you want to save this password?" bars at the top) suggesting to the user to re-evaluate all cells? Again, I am not suggesting that it’s worth it, but just wanted to throw in the thought.

Contributor

hmeine commented Jan 26, 2012

Great idea. I also think that the new notebook is much nicer (polished) than the 0.12 one, and that releasing it soon and as 0.13 would be warranted.

BTW: In case someone thinks that the polished appearance would be diminished by doubly-escaped HTML, how about detecting this and adding a notification (similar to modern browser’s "do you want to save this password?" bars at the top) suggesting to the user to re-evaluate all cells? Again, I am not suggesting that it’s worth it, but just wanted to throw in the thought.

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jan 26, 2012

Member

@minrk: I would love to chat on IRC about this one later today. I
will be around after lunch sometime.

On Wed, Jan 25, 2012 at 3:40 PM, Min RK
reply@reply.github.com
wrote:

@ellisonbg @fperez any further thoughts on this one?  I think it's the biggest bug in 0.12, and the fix has been outstanding for a over a month.

@fperez - there is a way to avoid the glitches - defining the bug into the notebook format, and incrementing the version, but I would put the costs of that (unreadability of 0.12.1 notebooks in 0.12) as much higher than the glitches themselves, which are trivially resolved by rerunning the notebook (works both directions).

I would vote for fixing the bug in the frontend (as done here), and pushing out 0.12.1 as a critical bugfix ASAP.


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

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

Member

ellisonbg commented Jan 26, 2012

@minrk: I would love to chat on IRC about this one later today. I
will be around after lunch sometime.

On Wed, Jan 25, 2012 at 3:40 PM, Min RK
reply@reply.github.com
wrote:

@ellisonbg @fperez any further thoughts on this one?  I think it's the biggest bug in 0.12, and the fix has been outstanding for a over a month.

@fperez - there is a way to avoid the glitches - defining the bug into the notebook format, and incrementing the version, but I would put the costs of that (unreadability of 0.12.1 notebooks in 0.12) as much higher than the glitches themselves, which are trivially resolved by rerunning the notebook (works both directions).

I would vote for fixing the bug in the frontend (as done here), and pushing out 0.12.1 as a critical bugfix ASAP.


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

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

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jan 26, 2012

Member

Hey guys,

On Thu, Jan 26, 2012 at 9:38 AM, Brian E. Granger
reply@reply.github.com
wrote:

@minrk: I would love to chat on IRC about this one later today.  I
will be around after lunch sometime.

I'll do my best, but I have a bit of a crazy day, so if I'm not there,
please proceed without me. We can summarize/decide later on the list.

Member

fperez commented Jan 26, 2012

Hey guys,

On Thu, Jan 26, 2012 at 9:38 AM, Brian E. Granger
reply@reply.github.com
wrote:

@minrk: I would love to chat on IRC about this one later today.  I
will be around after lunch sometime.

I'll do my best, but I have a bit of a crazy day, so if I'm not there,
please proceed without me. We can summarize/decide later on the list.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 26, 2012

Member

If we are doing 0.13 by March, maybe it doesn't make sense anymore to do 0.12.1, which we probably should have done a month ago, closer to when the issue came up.

Member

minrk commented Jan 26, 2012

If we are doing 0.13 by March, maybe it doesn't make sense anymore to do 0.12.1, which we probably should have done a month ago, closer to when the issue came up.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jan 26, 2012

Member

On Thu, Jan 26, 2012 at 11:27 AM, Min RK
reply@reply.github.com
wrote:

If we are doing 0.13 by March, maybe it doesn't make sense anymore to do 0.12.1, which we probably should have done a month ago, closer to when the issue came up.

That's my sense at this point, but let's see how that fits with
Brian's current work path, as well as other outstanding PRs. We can
try to decide in the next day or two.

Member

fperez commented Jan 26, 2012

On Thu, Jan 26, 2012 at 11:27 AM, Min RK
reply@reply.github.com
wrote:

If we are doing 0.13 by March, maybe it doesn't make sense anymore to do 0.12.1, which we probably should have done a month ago, closer to when the issue came up.

That's my sense at this point, but let's see how that fits with
Brian's current work path, as well as other outstanding PRs. We can
try to decide in the next day or two.

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jan 26, 2012

Member

I think a 0.13 release in 2-3 weeks is probably a tad to early. But
let's see how it goes.

On Thu, Jan 26, 2012 at 11:33 AM, Fernando Perez
reply@reply.github.com
wrote:

On Thu, Jan 26, 2012 at 11:27 AM, Min RK
reply@reply.github.com
wrote:

If we are doing 0.13 by March, maybe it doesn't make sense anymore to do 0.12.1, which we probably should have done a month ago, closer to when the issue came up.

That's my sense at this point, but let's see how that fits with
Brian's current work path, as well as other outstanding PRs.  We can
try to decide in the next day or two.


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

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

Member

ellisonbg commented Jan 26, 2012

I think a 0.13 release in 2-3 weeks is probably a tad to early. But
let's see how it goes.

On Thu, Jan 26, 2012 at 11:33 AM, Fernando Perez
reply@reply.github.com
wrote:

On Thu, Jan 26, 2012 at 11:27 AM, Min RK
reply@reply.github.com
wrote:

If we are doing 0.13 by March, maybe it doesn't make sense anymore to do 0.12.1, which we probably should have done a month ago, closer to when the issue came up.

That's my sense at this point, but let's see how that fits with
Brian's current work path, as well as other outstanding PRs.  We can
try to decide in the next day or two.


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

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

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jan 27, 2012

Member

Reviewed on IRC by @ellisonbg, and merging now.

Member

minrk commented Jan 27, 2012

Reviewed on IRC by @ellisonbg, and merging now.

minrk added a commit that referenced this pull request Jan 27, 2012

Merge pull request #1206 from minrk/fixConsole
The notebook js transforms ANSI-escaped text to HTML.  The transformed HTML was previously persisted to ipynb files, rather than the true output.  This has been fixed, and notebooks with ANSI-colored output will need to be re-run for colored output (tracebacks) to be displayed correctly after this change.

@minrk minrk merged commit f3ee404 into ipython:master Jan 27, 2012

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1206 from minrk/fixConsole
The notebook js transforms ANSI-escaped text to HTML.  The transformed HTML was previously persisted to ipynb files, rather than the true output.  This has been fixed, and notebooks with ANSI-colored output will need to be re-run for colored output (tracebacks) to be displayed correctly after this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment