Add T20 ruff rule (print calls) #6849

brisvag · 2024-04-16T10:15:54Z

References and relevant issues

Tracking issue: #5589

Previous discussion: #6840

Description

As asked in #6840, I split out this more controversial rule from it, so we can discuss it separately.

brisvag · 2024-04-16T14:04:42Z

@jni, I switched to logging where I thought it made sense, keeping print + noqa only for the reasonable cases. Still not sure this is worth it, but at least it doesn't introduce big changes.

…print-rule

# References Tracking issue: #5589 # Description This PR adds a few rules to be checked by `ruff`. Some of them required no changes or very little; you can click on individual commits to see them one by one. ~The only one that needs discussion is `T20` (print statements) which required a few ignores.~ EDIT: moved it to #6849 The rules are: - "ASYNC": avoid some async-related pitfalls (no changes needed) - "EXE": ensure shebangs and executable permissions match (1 change) - "FA": ensure `from __future__ import annotations` is used correctly (1 change) - "LOG": ensure loggers are used correctly (no changes) I'm going through a few more, but might split out into separate PRs. Happy to split this one out as well. ~EDIT: depends on #6775~ No longer true, rebased on main and it's fine.

brisvag · 2024-04-25T12:33:54Z

@jni input appreciated!

codecov · 2024-05-03T10:19:52Z

Codecov Report

Attention: Patch coverage is 48.00000% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 92.43%. Comparing base (a9c25e6) to head (7478720).
Report is 5 commits behind head on main.

Files	Patch %	Lines
napari/__main__.py	50.00%	2 Missing ⚠️
napari/_qt/dialogs/qt_notification.py	0.00%	2 Missing ⚠️
napari/_qt/widgets/qt_theme_sample.py	0.00%	2 Missing ⚠️
napari/components/experimental/monitor/_monitor.py	0.00%	2 Missing ⚠️
napari/utils/perf/_patcher.py	60.00%	2 Missing ⚠️
napari/utils/notifications.py	66.66%	1 Missing ⚠️
napari/utils/perf/_timers.py	0.00%	1 Missing ⚠️
napari/utils/stubgen.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6849      +/-   ##
==========================================
- Coverage   92.45%   92.43%   -0.02%     
==========================================
  Files         614      613       -1     
  Lines       55166    55153      -13     
==========================================
- Hits        51002    50983      -19     
- Misses       4164     4170       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

napari/_tests/test_import_time.py

jni · 2024-05-05T06:48:57Z

This PR changes 11 prints to print # noqa, and 7 prints to logging/warn. (And note that for our purposes logging is in fact equivalent to print.) imho, that is a bit of a smell that the rule is too stringent and we should not activate it. I'm not actively going to block it but yeah, I don't love it.

Czaki · 2024-05-05T19:53:00Z

napari/utils/notifications.py

    except Exception:
-        print(
+        print(  # noqa: T201


I prefer to change this to logging.exception

@brisvag and this?

isn't this a place where we want the user to see it? So maybe warnings?

Looking at this code, I think that warning may not be rendered.

But warnings should show up as popups right?

Lets do this this way

Or maybe not. The logging.exception will also print exception information. There is no equivalent for warnings. If you prefer warning, then we should print stacktrace somehow.

ok, logging is fine by me then. We should really get a custom handler going afterwards :)

but do we still raise after? I guess so?

See comment above raise.

Czaki · 2024-05-05T19:57:20Z

napari/_qt/dialogs/qt_notification.py

@@ -405,9 +405,11 @@ def _debug_tb(tb):
    QApplication.processEvents()
    QApplication.processEvents()
    with event_hook_removed():
-        print("Entering debugger. Type 'q' to return to napari.\n")
+        print(  # noqa: T201


loggin.info?

Not sure how to deals with these cause I'm not familiar with this module. Isn't this print here on purpose so the traceback can be shown to the user?

napari/_tests/test_import_time.py

Czaki · 2024-05-05T20:00:26Z

(And note that for our purposes logging is in fact equivalent to print.)

And we should change this. We should implement a custom logger grabber and GUI to inspect logs from application.

jni · 2024-05-06T00:11:35Z

And we should change this. We should implement a custom logger grabber and GUI to inspect logs from application.

Ok I'm convinced. Maybe changing the remaining prints to logs then makes sense.

brisvag · 2024-05-06T09:07:16Z

This PR changes 11 prints to print # noqa, and 7 prints to logging/warn.

It's more like "at least half of our prints shouldn't be there" :P Also, this rule is more to prevent accidentally adding prints to places where they shouldn't be; the few prints that should be there (e.b the outputs of the cli) are already there and are unlikely to change.

Agree it's not a super-important rule, but I think it does more good than bad. Agree we should have a log handler we grab everywhere! It could be done as part of this PR as well. @Czaki I'm not sure I follow the GUI comment; can you elaborate?

Czaki · 2024-05-06T10:33:37Z

Agree it's not a super-important rule, but I think it does more good than bad. Agree we should have a log handler we grab everywhere! It could be done as part of this PR as well. @Czaki I'm not sure I follow the GUI comment; can you elaborate?

Something like icy Output tab:

Or console log in ImageJ. So place where it will be accessible to user who launch napari without a terminal.

It may be even smarter (like runtime level filtering, or selection of which logger output should be published).

jni · 2024-05-06T13:36:46Z

I don't think the logging console needs to be part of this PR (indeed, maybe not even this repo 😂), but rather, change every remaining print to a log. What do you think @brisvag @Czaki ?

Czaki · 2024-05-06T14:14:05Z

I don't think the logging console needs to be part of this PR (indeed, maybe not even this repo 😂)

I think it should be part of the default installation napari[all], but maybe outside this repo. But if @brisvag want, it could be even part of this PR.

but rather, change every remaining print to a log.

No. Output of napari --info should be printed using print, not logger.

brisvag · 2024-05-06T14:33:24Z

I think it should be part of the default installation napari[all], but maybe outside this repo. But if @brisvag want, it could be even part of this PR.

Definitely not this PR, but I agree that it would be nice to just have it somewhere. As long as we use logging properly, we should be able to do this at a later stage.

No. Output of napari --info should be printed using print, not logger.

Yeah, and a couple other things!

@Czaki: what's the general idea behing using warnings.warn vs logging.warning? We seem to use them randomly, maybe we should always use log?

Czaki · 2024-05-06T14:45:09Z

@Czaki: what's the general idea behing using warnings.warn vs logging.warning? We seem to use them randomly, maybe we should always use log?

warnings are shown as modal dialog in the bottom-right corner and logging is not displayed.

I think that warning is something that should be immediately displayed to the user and logging for later access.

brisvag · 2024-05-06T15:58:14Z

But don't we want the warnings to the user to also be displayed in the logs? Wouldn't it be cleaner to take all logs from level WARN or more and push them as notifications, instead of using two different systems? I also just confused about why python offers these two systems in the standard library...

Czaki · 2024-05-06T16:04:29Z

For example, warning by default provides deduplication of events (with well, known bug). And logging push to logs every event that happens.

Warnings were introduced in python 2.1 and logging in python 2.3 (based on documentation).

Warnings are global, logging offers the option to have multiple loggers with different settings.

I see that there are similar, but not equivalent. I'm not sure if we want to change every logging into notification.

…print-rule

brisvag · 2024-05-06T16:23:48Z

I changed 1 more print into log, but I think the rest should remain noqa because they are all special cases. Let me know if you disagree!

I also moved the import time to a benchmark based on the implementation at https://github.com/proost/pandas/blob/master/asv_bench/benchmarks/package.py. Let's see if it works :)

github-actions · 2024-05-06T19:50:09Z

The Qt benchmark run requested by PR #6849 (09eb7e3 vs 1dcbade) has finished with status 'failure'. See the CI logs and artifacts for further details.
The non-Qt benchmark run requested by PR #6849 (09eb7e3 vs 1dcbade) has finished with status 'success'. See the CI logs and artifacts for further details.

github-actions · 2024-05-06T20:10:32Z

The Qt benchmark run requested by PR #6849 (cd2d5da vs 1dcbade) has finished with status 'success'. See the CI logs and artifacts for further details.
The non-Qt benchmark run requested by PR #6849 (cd2d5da vs 1dcbade) has finished with status 'success'. See the CI logs and artifacts for further details.

Czaki · 2024-05-07T10:56:01Z

@brisvag It looks like the last merge to main introduced merge conflict

…print-rule

jni · 2024-05-15T11:24:59Z

napari/utils/perf/_patcher.py

            # We don't stop on error because if you switch around branches
            # but keep the same config file, it's easy for your config
            # file to contain targets that aren't in the code.
-            print(f'Patcher: [ERROR] {exc}')
+            logging.exception('Something went wrong while patching')


Should we not log the exception message itself?

logging.exception logs the exception without passing it explicitly by doing some magic inspection of the stack trace!

🤯

Can you add a comment above it pointing that out? Cos that is just batshit crazy. 😂

jni · 2024-05-23T14:02:13Z

LOL @brisvag literally the day after you merge this, with much resistance from me:

~/projects/napari on monitor-fps(✗) at 0:00:15
$ git commit -a -m "Fix docstring indentation"
ruff-format..............................................................Passed
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

napari/_qt/qt_viewer.py:1070:13: T201 `print` found
napari/_qt/qt_viewer.py:1074:13: T201 `print` found
napari/_qt/qt_viewer.py:1080:9: T201 `print` found
Found 3 errors.

😅

github-actions bot added tests Something related to our tests qt Relates to qt labels Apr 16, 2024

brisvag mentioned this pull request Apr 16, 2024

Add a few ruff lint rules: ASYNC, EXE, FA and LOG #6840

Merged

add and apply T20

7ceac6a

brisvag force-pushed the ci/ruff-print-rule branch from 848607a to f1b210d Compare April 16, 2024 14:00

use logging where possible

f1b210d

brisvag added 3 commits April 16, 2024 16:06

do not special case main

a8cb3f9

ok fine print it is

b2ddff2

Merge branch 'main' of https://github.com/napari/napari into ci/ruff-…

ea5e190

…print-rule

brisvag marked this pull request as ready for review April 17, 2024 12:31

brisvag added ci Continuous integration maintenance PR with maintance changes, labels Apr 17, 2024

brisvag added this to the 0.5.0 milestone Apr 17, 2024

brisvag requested review from Czaki and jni April 17, 2024 12:32

brisvag mentioned this pull request Apr 18, 2024

Decision about ruff rules #5589

Open

12 tasks

Merge branch 'main' into ci/ruff-print-rule

25fd548

Czaki approved these changes May 3, 2024

View reviewed changes

jni reviewed May 5, 2024

View reviewed changes

napari/_tests/test_import_time.py Outdated Show resolved Hide resolved

Czaki reviewed May 5, 2024

View reviewed changes

brisvag added 2 commits May 6, 2024 18:10

Merge branch 'main' of https://github.com/napari/napari into ci/ruff-…

de3ac0f

…print-rule

move imprt time test to benchmark

1f00912

fix remaining issues

ff108e7

brisvag added run-benchmarks Add this label to trigger a full benchmark run in a PR and removed run-benchmarks Add this label to trigger a full benchmark run in a PR labels May 6, 2024

switch test to caplog

b2eceec

brisvag removed the run-benchmarks Add this label to trigger a full benchmark run in a PR label May 7, 2024

brisvag added 2 commits May 7, 2024 14:30

Merge branch 'main' of https://github.com/napari/napari into ci/ruff-…

fe95320

…print-rule

switch to logging.exception

a2dd06c

brisvag mentioned this pull request May 7, 2024

Add a custom log handler #6900

Draft

brisvag requested review from Czaki and jni May 13, 2024 12:34

Czaki approved these changes May 13, 2024

View reviewed changes

jni reviewed May 15, 2024

View reviewed changes

jni approved these changes May 15, 2024

View reviewed changes

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

add comment for logging.exception

7478720

brisvag merged commit 98c2e0f into napari:main May 22, 2024
34 checks passed

github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add T20 ruff rule (print calls) #6849

Add T20 ruff rule (print calls) #6849

brisvag commented Apr 16, 2024

brisvag commented Apr 16, 2024

brisvag commented Apr 25, 2024

codecov bot commented May 3, 2024 •

edited

jni commented May 5, 2024

Czaki May 5, 2024

Czaki May 7, 2024

brisvag May 7, 2024

Czaki May 7, 2024

brisvag May 7, 2024

Czaki May 7, 2024

Czaki May 7, 2024

brisvag May 7, 2024

brisvag May 7, 2024

Czaki May 7, 2024

Czaki May 5, 2024

brisvag May 7, 2024

Czaki commented May 5, 2024

jni commented May 6, 2024

brisvag commented May 6, 2024

Czaki commented May 6, 2024

jni commented May 6, 2024

Czaki commented May 6, 2024

brisvag commented May 6, 2024

Czaki commented May 6, 2024

brisvag commented May 6, 2024

Czaki commented May 6, 2024

brisvag commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

Czaki commented May 7, 2024

jni May 15, 2024

brisvag May 15, 2024

jni May 15, 2024

jni commented May 23, 2024

Add T20 ruff rule (print calls) #6849

Add T20 ruff rule (print calls) #6849

Conversation

brisvag commented Apr 16, 2024

References and relevant issues

Description

brisvag commented Apr 16, 2024

brisvag commented Apr 25, 2024

codecov bot commented May 3, 2024 • edited

Codecov Report

jni commented May 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki commented May 5, 2024

jni commented May 6, 2024

brisvag commented May 6, 2024

Czaki commented May 6, 2024

jni commented May 6, 2024

Czaki commented May 6, 2024

brisvag commented May 6, 2024

Czaki commented May 6, 2024

brisvag commented May 6, 2024

Czaki commented May 6, 2024

brisvag commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

Czaki commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jni commented May 23, 2024

codecov bot commented May 3, 2024 •

edited