Push variables to console when instantiated #5210

davidpross · 2022-10-12T21:17:16Z

Description

Currently, if update_console is called on viewer object before the console is instantiated the function does nothing. With this PR instead references to the requested variables are added to a console_backlog list attribute on qt_viewer. If and when console is instantiated these variables are pushed.

Unsure if this related PR is moving forward and whether it would solve this case.

Type of change

Bug-fix (non-breaking change which fixes an issue)

References

Closes #4098

How has this been tested?

Ran unit tests for napari/_qt. There were some errors but looked unrelated to my changes.
Added unit test based on existing test_update_console

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

codecov · 2022-10-12T21:52:24Z

Codecov Report

Merging #5210 (7eeb664) into main (49fee1d) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5210      +/-   ##
==========================================
+ Coverage   89.82%   90.01%   +0.18%     
==========================================
  Files         610      618       +8     
  Lines       52131    52769     +638     
==========================================
+ Hits        46825    47498     +673     
+ Misses       5306     5271      -35

Impacted Files	Coverage Δ
napari/_qt/qt_viewer.py	`75.80% <100.00%> (+2.12%)`	⬆️
napari/_tests/test_advanced.py	`100.00% <100.00%> (ø)`
napari/viewer.py	`88.46% <100.00%> (+0.22%)`	⬆️

... and 89 files with indirect coverage changes

psobolewskiPhD · 2022-10-13T17:18:53Z

This works quite nicely for me locally!
While you have this PR open, can you also fix the example that the issue refers to?
It's making two viewers, in a rather nonsensical fashion, as noted in the original issue:
#4098 (comment)
Worse yet, it means using the example to test your fix doesn't work!

Just change:

napari/examples/update_console.py

Line 22 in c305b46

image_layer = napari.view_image(photographer, name='photographer')

from napari.view_image to viewer.add_image
(Also, I'm not sure what the deal is with printing in the doc string, it doesn't print anything, but with your fix one can access polygons. You could delete that I guess.)

I think fixing this is useful, even if I personally think the console should inherit from scripts automagically. That's what napari/napari-console#18 is trying to do, which would make this obsolete, but it's still WIP, so in the meantime this is nice.

davidpross · 2022-10-15T02:59:55Z

This works quite nicely for me locally!
While you have this PR open, can you also fix the example that the issue refers to?

Thanks for checking it out, I added a fix for the example to not open two viewers and also printed the updated shapes coordinates after the window closes.

Czaki · 2022-10-20T08:41:20Z

Aaybe it will be also nice to always push np to the console to not require type import numpy as np

psobolewskiPhD · 2022-10-20T09:03:05Z

Aaybe it will be also nice to always push np to the console to not require type import numpy as np

On one hand I agree, it's handy, saves keystrokes. On the other it seems potentially dangerous because some snippets will work as if by magic without an explicit import and a new python user may then be confused when in ipython/jupyter they don't. I've found the console very nice to learn and explore, as well as show python to people, so I think i'd prefer that such behavior not be included.
Perhaps it's a new PR to make some sort of console init preference to import on open?

brisvag · 2022-11-02T14:07:46Z

On the other it seems potentially dangerous because some snippets will work as if by magic without an explicit import and a new python user may then be confused when in ipython/jupyter they don't.

In the past I dealt with similar issue by having the console print a header where it informs that some things have been imported for the user, something like:

--- napari console ---
The napari viewer is accessible from the `viewer` variable.
Additionally, numpy was imported as `np` and ...
---

psobolewskiPhD · 2022-11-02T16:23:56Z

I like that a lot. Maybe 2nd PR though? Could be some discussion as to what should be auto-imported and how the message should look.

hanjinliu · 2022-11-03T11:26:43Z

I think the safer but more useful way is to execute the ipython startup files on launching the napari console (see here as an example if you are not familiar with).

brisvag · 2022-11-04T10:18:29Z

Yes, that's also great, though not really equivalent (we still have the viewer variable, for example, which is a napari-only thing)

jni · 2022-12-13T07:05:06Z

I think napari/napari-console#18 would indeed solve this. I'd be in favour of pushing on that one but I'm not sure whether @Carreau is around to push it over the line... I'm not opposed to merging this though because the implementation is so simple. I only have a minor question which is that it seems a bit pointless to reset the blank value to None instead of always using the empty list? ie you would save an if clause if you just set it to the empty list?

kephale · 2022-12-13T17:04:05Z

napari/_tests/test_advanced.py

@@ -180,6 +180,34 @@ def test_update_console(make_napari_viewer):
        del viewer.window._qt_viewer.console.shell.user_ns[k]


+@pytest.mark.filterwarnings("ignore::DeprecationWarning:jupyter_client")
+def test_update_lazy_console(make_napari_viewer):


I am trying to understand if this test will still do its job if the startup time for console is decreased to the point that it is loaded when update_console is called.

With the lazy instantiation the console shouldn't begin starting up until the assertion on L196 below.

davidpross · 2022-12-16T00:28:56Z

I only have a minor question which is that it seems a bit pointless to reset the blank value to None instead of always using the empty list? ie you would save an if clause if you just set it to the empty list?

I think my last commit implemented your suggestion but let me know if I misinterpreted it.

Carreau · 2022-12-21T13:27:02Z

I think napari/napari-console#18 would indeed solve this. I'd be in favor of pushing on that one but I'm not sure whether @Carreau is around to push it over the line...

Yeah, sorry I'm been a bit under the wether over the past two weeks.

Seeing that napari console is not super active, have considered having a mono-repo under napari, that has both napari and napari console, it would make it a bit easier to manage all in the same place ?

davidpross · 2023-02-24T21:38:58Z

Added description for when creating a weakref fails, also made suggested updates. In addition on further reflection removed special handling for list objects because if a list or tuple is passed it is supposed to only contain strings of variable names. So weakrefs are created if possible for the values of any dictionary passed to update_console when console hasn't been instantiated. A weakref will be created for something like a numpy array but not a Python list. So there could still be some strong references if you passed something like update_console({'obj_list': [obj1, obj2, obj3]})

jni · 2023-02-25T12:18:29Z

napari/_qt/qt_viewer.py

+        Args:
+            value (Object|weakref): Can be a weakref
+
+        Returns:
+            Object|None: Returns object or None if weakref is dead.


@davidpross can you use numpydoc formatting here? 🙏 (and above). Otherwise, nice, thanks! 😊

Updated docstrings, thanks for all the help with these changes.

kephale · 2023-04-17T16:24:12Z

@Carreau just a gently nudge for review on this one. Thank you!

Carreau · 2023-04-17T16:40:59Z

@Carreau just a gently nudge for review on this one. Thank you!

That looks good to me, there will always be edge case we can refine later.

for more information, see https://pre-commit.ci

jni · 2023-04-27T09:20:38Z

napari/_qt/qt_viewer.py

+                                }
+                            )
+                        else:
+                            self.console.push(i)


@davidpross I think this line isn't covered by tests? Ditto for line 508. tbh I don't understand the usage that would get us there. Could you elaborate on that and maybe add a test? I think this is very close to merging otherwise! 🙏

Good catch on the test coverage. It turns out that passing a variable name as a string or list of variable names was not working in the context of a console that is not yet instantiated. I added code from here to handle these cases and updated the unit tests accordingly. Also the console_backlog will now always be a list of dicts.

jni · 2023-04-27T10:41:37Z

btw I've pushed some minor formatting fixes so please git pull before making further changes!

for more information, see https://pre-commit.ci

Currently, if [`update_console`](https://github.com/napari/napari/blob/7117d22e4baa740864762454911dc5ed09764ccc/napari/viewer.py#L88) is called on viewer object before the console is instantiated the function does nothing. With this PR instead references to the requested variables are added to a `console_backlog` list attribute on qt_viewer. If and when console is instantiated these variables are pushed. Unsure if [this related PR](napari/napari-console#18) is moving forward and whether it would solve this case. - [x] Enhancment Closes #4098 - [x] Ran unit tests for `napari/_qt`. There were some errors but looked unrelated to my changes. - [x] Added unit test based on existing `test_update_console` - [x] My PR is the minimum possible work for the desired functionality - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] I have added tests that prove my fix is effective or that my feature works --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

davidpross added 2 commits October 12, 2022 13:34

Push variables to console when instantiated

487c6e8

Add unit test for update console

736f46d

github-actions bot added qt Relates to qt tests Something related to our tests labels Oct 12, 2022

github-actions bot assigned davidpross Oct 12, 2022

Merge branch 'main' into update-console

2b362e0

Merge branch 'main' into update-console

aef2ca7

davidpross and others added 3 commits October 14, 2022 19:50

Fix example opening two viewers

753f2cd

Reset console_backlog after push

2447197

Merge branch 'main' into update-console

94d3381

davidpross added 2 commits October 15, 2022 11:00

Merge branch 'main' into update-console

cfc8870

Merge branch 'main' into update-console

1034062

psobolewskiPhD requested a review from Carreau October 17, 2022 20:23

Merge branch 'main' into update-console

f7503a2

Merge branch 'main' into update-console

46d1942

kephale reviewed Dec 13, 2022

View reviewed changes

davidpross and others added 2 commits December 15, 2022 15:08

Merge branch 'napari:main' into update-console

16d362a

Reset _console_backlog to empty list

fd50eb9

jni reviewed Feb 25, 2023

View reviewed changes

Change docstring to numpy formatting.

54ba014

davidpross and others added 2 commits April 26, 2023 14:40

Merge 'upstream/main' into update-console

254c3d8

[pre-commit.ci] auto fixes from pre-commit.com hooks

68c97cc

for more information, see https://pre-commit.ci

jni added the enhancement label Apr 27, 2023

jni added 2 commits April 27, 2023 19:16

Remove stray print statement

2ba9776

Reformat docstring to conform to PEP257

4e979de

jni approved these changes Apr 27, 2023

View reviewed changes

jni reviewed Apr 27, 2023

View reviewed changes

jni and others added 7 commits April 29, 2023 12:15

Appease the Ruff RET505 gods

96335e1

[pre-commit.ci] auto fixes from pre-commit.com hooks

f14411c

for more information, see https://pre-commit.ci

Remove obsolete comment

6dcb34e

Lookup variables by name in caller's frame

cdfe433

[pre-commit.ci] auto fixes from pre-commit.com hooks

db648f9

for more information, see https://pre-commit.ci

Fix ruff failures

78466ce

Update error type in test

7eeb664

jni added the ready to merge Last chance for comments! Will be merged in ~24h label May 26, 2023

melonora mentioned this pull request Jun 15, 2023

v0.4.18 - release process #5911

Closed

Czaki added this to the 0.4.18 milestone Jun 15, 2023

Czaki merged commit bea22f5 into napari:main Jun 15, 2023
35 checks passed

Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Jun 15, 2023

davidpross deleted the update-console branch June 20, 2023 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push variables to console when instantiated #5210

Push variables to console when instantiated #5210

davidpross commented Oct 12, 2022

codecov bot commented Oct 12, 2022 •

edited

Loading

psobolewskiPhD commented Oct 13, 2022

davidpross commented Oct 15, 2022

Czaki commented Oct 20, 2022

psobolewskiPhD commented Oct 20, 2022

brisvag commented Nov 2, 2022

psobolewskiPhD commented Nov 2, 2022

hanjinliu commented Nov 3, 2022

brisvag commented Nov 4, 2022

jni commented Dec 13, 2022

kephale Dec 13, 2022

davidpross Dec 15, 2022

davidpross commented Dec 16, 2022

Carreau commented Dec 21, 2022

davidpross commented Feb 24, 2023

jni Feb 25, 2023

davidpross Feb 25, 2023

kephale commented Apr 17, 2023

Carreau commented Apr 17, 2023

jni Apr 27, 2023

davidpross Apr 30, 2023

jni commented Apr 27, 2023

Push variables to console when instantiated #5210

Push variables to console when instantiated #5210

Conversation

davidpross commented Oct 12, 2022

Description

Type of change

References

How has this been tested?

Final checklist:

codecov bot commented Oct 12, 2022 • edited Loading

Codecov Report

psobolewskiPhD commented Oct 13, 2022

davidpross commented Oct 15, 2022

Czaki commented Oct 20, 2022

psobolewskiPhD commented Oct 20, 2022

brisvag commented Nov 2, 2022

psobolewskiPhD commented Nov 2, 2022

hanjinliu commented Nov 3, 2022

brisvag commented Nov 4, 2022

jni commented Dec 13, 2022

kephale Dec 13, 2022

Choose a reason for hiding this comment

davidpross Dec 15, 2022

Choose a reason for hiding this comment

davidpross commented Dec 16, 2022

Carreau commented Dec 21, 2022

davidpross commented Feb 24, 2023

jni Feb 25, 2023

Choose a reason for hiding this comment

davidpross Feb 25, 2023

Choose a reason for hiding this comment

kephale commented Apr 17, 2023

Carreau commented Apr 17, 2023

jni Apr 27, 2023

Choose a reason for hiding this comment

davidpross Apr 30, 2023

Choose a reason for hiding this comment

jni commented Apr 27, 2023

codecov bot commented Oct 12, 2022 •

edited

Loading