bugGNU Octave - Bugs: bug #65644, jupyter-notebook tests fail if a...

 
 

bug #65644: jupyter-notebook tests fail if a figure is already open

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Fri 26 Apr 2024 02:57:57 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  9.3.0 (current stable) Planned Release:  9.3.0 (current stable)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 05 Jun 2024 12:06:55 AM UTC, comment #10: 

looks like jwe did the version bump to 9.3.0.  moved the NEWS notes over to the 9.3 section in:
https://hg.savannah.gnu.org/hgweb/octave/rev/3ee1754876bf

Nicholas Jankowski <nrjank>
Group Member
Tue 04 Jun 2024 11:44:25 PM UTC, comment #9: 

This looks good.  I made small change of using newlines between files in the commit message and using two spaces at the start of sentences in documentation such as NEWS.9.md.  I checked your changeset in on the stable branch here https://hg.savannah.gnu.org/hgweb/octave/rev/7dac60c39ad0.

Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Tue 04 Jun 2024 09:45:00 PM UTC, comment #8: 

v2 patch attached that puts the figure creation, code execution, and cleanup actions in an unwind_protect block.  probably could use a test to ensure those cleanups actually happen.

turns out the default visibility retention was actually at the .tst file level and the cases where it wasn't reset properly was due to me terminating a debug sequence.

(file #56137)

Nicholas Jankowski <nrjank>
Group Member
Tue 04 Jun 2024 09:21:58 PM UTC, comment #7: 

patch actually attached...

(file #56136)

Nicholas Jankowski <nrjank>
Group Member
Tue 04 Jun 2024 09:03:15 PM UTC, comment #6: 

confirming that simply changing

    if (exist ("newFig") && isempty (get (newFig, "children")))



    if (exist ("newFig") && ishandle (newFig) && isempty (get (newFig, "children")))


eliminates the error if newFig is closed by the eval'd code.

since newFig is supposed to hold any test figures created, there's no good reason for a 'close all hidden' line in the test code, even though it no longer errors.  to avoid inadvertently closing existing figures when running the test, we should remove that line. it doesn't seem to cause any problems.

the attached patch makes those changes against stable.

there's also a FIXME note that much of the cleanup should be placed inside a unwind_protect/ cleanup  block so it gets run on an error.  e.g., i noticed the subject error kept figures set to default visibility off and left that unrestored. I haven't done that, but it would probably be worth doing.

marking as patch sumbitted.

Nicholas Jankowski <nrjank>
Group Member
Tue 04 Jun 2024 08:46:46 PM UTC, comment #5: 

looks like the intent of that code is to:

1 - check for existing figs, capture their fig numbers in fig_ids
2 - create a new blank fig at # n+1
3 - run whatever plotting code is in `__code__`
4 - check if the new black fig was used or not the failing get
5 - if so, delete it
6 - check remaining figures against fig_ids and delete anything new.
7 - lots of manual cleanup actions to remove tempfiles.


simply removing the 'close all hidden' line avoids the error, and test jupyter-notebook.tst passes whether or not an existing figure window is present. 

it seems as long as the eval'd code doesn't close the new figure the rest of the code runs without issue. e.g., putting a close(gcf) at the end of the code snippet causes the same error (as that closes the newFig figure), but a close(1) closing the existing window but leaving the newFig figure runs without error.

I'm not familiar enough with jupyter_notebook() to know how arbitrary code needs to be permitted to be.  probably we could add an ishandle(newFig) check to that conditional before the get(newFig) check. i'll play with that a bit.

Nicholas Jankowski <nrjank>
Group Member
Tue 04 Jun 2024 08:01:20 PM UTC, comment #4: 

if a figure already exists, newFig is supposed to be created at line 383 if it detects an existing figure (assigned to the next figure number)

but then after the following:

output_lines = obj.evalCode (strjoin (nbcell.source));

ishandle(newFig) returns false. (the variable still exists, but it seems it's just a double).

that triggers the get error a few lines later at the line you indicated, as get(newFig, ...) isn't querying a graphics handle object.

the code being sent to evalCode is:

    "% Create an RGB image with no title or labels\n",
    "close all hidden\n",
    "image = zeros(300,400,3);\n",
    "image(1:100,:,1) = 0.9;  % Red\n",
    "image(101:200,:,2) = rand(100,400);  % Green\n",
    "image(201:300,:,3) = 0.9;  % Blue\n",
    "imshow(image)"


looking inside evalCode,  it runs eval on that code block, which kills all open windows with the 'close all hidden' line. that also kills the figure newFig was pointing to. so that explains the later false return on ishandle.

 But i'm not sure why there are no problems if there are no existing figures. perhaps the issue is that later it automagically works out if the figure number was expected to be 1, which it wasn't if there was an existing figure.

Nicholas Jankowski <nrjank>
Group Member
Tue 04 Jun 2024 03:32:45 AM UTC, comment #3: 

Confirmed on current development branch.  I think the problem is with the jupyter_notebook code rather than with the test itself.  I extracted the test code so that I could run it.  The error message is


error: get: invalid handle (= 2)
error: called from
    run at line 399 column 11
    run_all at line 475 column 9
    tst_jup at line 16 column 4


The code in jupyter_notebook.m is


      ## If there are existing plots and newFig is empty, delete it.
      if (exist ("newFig") && isempty (get (newFig, "children")))


Apparently newfig does not exist as a graphics handle.

Rik <rik5>
Group administrator
Fri 26 Apr 2024 04:17:57 AM UTC, comment #2: 

@Anonymous - i'm not sure what exactly you're suggesting.  I see those three lines in the test, but the test still fails.

Nicholas Jankowski <nrjank>
Group Member
Fri 26 Apr 2024 04:04:42 AM UTC, comment #1: 


visibility = get (0, "defaultfigurevisible");

set (0, "defaultfigurevisible", "off");

set (0, "defaultfigurevisible", visibility);


No error.


Anonymous
Fri 26 Apr 2024 02:57:57 AM UTC, original submission:  

as noticed over on bug #65641 when a test accidentally left a figure window up during the test suite,  that window being present caused a jupyter-notebook test to fail. This can be verified with just testing jupyter-notebook.

with no figures open:

close all
test jupyter-notebook.tst
PASSES 4 out of 4 tests



close all
figure
test jupyter-notebook.tst
***** testif HAVE_RAPIDJSON
 visibility = get (0, "defaultfigurevisible");
 toolkit = graphics_toolkit ();
 unwind_protect
   if (! strcmp (graphics_toolkit (), "qt"))
     try
       graphics_toolkit ("gnuplot");
     catch
       ## The system doesn't support gnuplot for drawing hidden
       ## figures.  Just return and have test marked as passing.
       return;
     end_try_catch
   endif
   set (0, "defaultfigurevisible", "off");

   n = jupyter_notebook (fullfile ("octave_kernel.ipynb"));
   n.run_all ();

   ## Test embedding images
   assert (n.notebook.cells{3}.outputs{1}.output_type, "display_data")
   assert (isfield (n.notebook.cells{3}.outputs{1}.data, "image/png"));
   assert (getfield (n.notebook.cells{3}.outputs{1}.data, "text/plain"),
           {"<IPython.core.display.Image object>"});

   ## Test running non-code cells
   markdown_cell = n.notebook.cells{1};
   n.run (1);
   assert (markdown_cell, n.notebook.cells{1});

   ## Test embedding textual output
   assert (n.notebook.cells{6}.outputs{1}.output_type, "stream")
   assert (n.notebook.cells{6}.outputs{1}.name, "stdout");
 unwind_protect_cleanup
   set (0, "defaultfigurevisible", visibility);
   graphics_toolkit (toolkit);
 end_unwind_protect
!!!!! test failed
get: invalid handle (= 2)


Nicholas Jankowski <nrjank>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #56137:  jupyter_figure_error_bug65644_V2.patch added by nrjank (7KiB - application/octet-stream - patch includes unwind protect.)
file #56136:  jupyter_figure_error_bug65644.patch added by nrjank (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-06-04 rik5 Open/ClosedOpen Closed
        Fixed Release9.2.0 9.3.0 (current stable)
        Planned Release9.2.0 9.3.0 (current stable)
    2024-06-04 rik5 StatusPatch Submitted Fixed
        Planned ReleaseNone 9.2.0
    2024-06-04 nrjank Attached File- Added jupyter_figure_error_bug65644_V2.patch, #56137
    2024-06-04 nrjank Attached File- Added jupyter_figure_error_bug65644.patch, #56136
    2024-06-04 nrjank StatusConfirmed Patch Submitted
        Fixed ReleaseNone 9.2.0
    2024-06-04 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-3e34.
    Corresponding source code