Notebook flake fixes.

[ceball]

Following on from #2091, I did some more work on a notebook flakes tool, and ran it on examples/. This PR is the result.

Notes:

• examples/user_guide/Deploying_Bokeh_Apps.ipynb: loop comes up as being an undefined name; is it?
• examples/user_guide/Plots_and_Renderers.ipynb: holoviews.plotting.mpl is imported near the start, but is unused. I made it be used. You might not like the change. An alternative supported by the flakes tool would be to add #noqa: reason to the holoviews.plotting.mpl import, and reverting the change I've made here.

[philippjfr]

Thanks for catching this, the IOLoop import must have gotten lost somewhere.

[philippjfr]

It used to be:

from tornado.ioloop import IOLoop
loop = IOLoop.current()

I wonder if this is needed though or if we can just call server.show() right after server.start().

[jbednar]

An alternative would be to do from holoviews.plotting import mpl as hpm at the top, then hpm.MPLRenderer.instance here...

[philippjfr]

I've shortened it to hv.plotting.mpl.MPLRenderer, I'm guessing that won't make the flakes tool happy but I'm personally not entirely convinced that level of strictness is appropriate for a notebook. Missing or completely unused (i.e. pointless) imports should be fixed, but I don't think we should start making the notebook purposefully verbose or add #noqa flags to satisfy it.

What's the flake tool called and can I start using it myself? I'd like to make a release checklist which would include things like running nbsmoke and this flake tool.

[jlstevens]

Looks good to me. I agree we do not want to have #noqa in notebooks or as you say, make notebooks more verbose than necessary just to satisfy flake checks.

Merging.

[ceball]

I'm personally not entirely convinced that level of strictness is appropriate for a notebook. Missing or completely unused (i.e. pointless) imports should be fixed, but I don't think we should start making the notebook purposefully verbose or add #noqa flags to satisfy it.

I agree we do not want to have #noqa in notebooks or as you say, make notebooks more verbose than necessary just to satisfy flake checks.

The intent of supporting # noqa: explanation in nbsmoke is to encourage genuinely unavoidable or genuinely desirable 'mysterious imports' (i.e. ones where there's no subsequent usage of the name you've imported) to be clarified. E.g. if you're importing something for its side effects, it's very helpful to inform the reader of that. I guess I hope there wouldn't be such imports in the first place, but no doubt they make sense in places.

Anyone got a better suggestion?

And how do you feel about the above, @jbednar?

[ceball]

Unfortunately I made a manual error here when dealing with the flakes caught by nbsmoke - xr is used. I'll make a new PR (probably alongside doing python 2 lint checks).

[philippjfr]

Thanks!

[philippjfr]

if you're importing something for its side effects, it's very helpful to inform the reader of that.

That's a good point and given that there seems to be exactly one such occurrence in the code I won't object marking it explicitly.

[ceball]

What's the flake tool called and can I start using it myself? I'd like to make a release checklist which would include things like running nbsmoke and this flake tool.

It's nbsmoke: pytest --nbsmoke-lint (rather than pytest --nbsmoke-run - it was my first pytest plugin so I didn't do a great job with the argument names/handling). However, note that you'd need a recent version of nbsmoke (to get recent improvements to flake checking when there's magics involved). And also note that I am currently running nbsmoke lint checks on holoviews, pyviz, datashader to test nbsmoke itself :) Once that's done, I hope all our projects that have notebooks could use nbsmoke flakes checking on travis. (I think that's preferable to running it as part of a release checklist, but you probably know better than I do, as I've never released holoviews...)

[philippjfr]

Once that's done, I hope all our projects that have notebooks could use nbsmoke flakes checking on travis.

No objection from me, as long as the checks aren't any more strict than flake8 --ignore=E,W,F999,F405.

[jbednar]

how do you feel about the above, @jbednar?

I'd have to see examples of where noqa in notebooks was really necessary. If we can avoid it altogether, I think we should, because notebooks are generally for showing people how things work, and any obscure syntax like that will confuse people. Of course, adding bogus actual statements like 0*val also confuses people, so it's a judgement call whether noqa is better if it comes down to that.

[ceball]

No objection from me, as long as the checks aren't any more strict than flake8 --ignore=E,W,F999,F405.

It uses pyflakes, so the idea is it should "never complain about style, and it will try very, very hard to never emit false positives."

[ceball]

I'd have to see examples of where noqa in notebooks was really necessary. If we can avoid it altogether, I think we should, because notebooks are generally for showing people how things work, and any obscure syntax like that will confuse people. Of course, adding bogus actual statements like 0*val also confuses people, so it's a judgement call whether noqa is better if it comes down to that.

By 'avoid it altogether', do you mean, 'avoid importing something that's not explicitly used, so noqa is never necessary in the first place', or do you mean, 'avoid adding noqa where something is imported but not explicitly used, and instead shut the linter up some other way'? (Or something else?)

[jlstevens]

Personally I would prefer it if we just make sure our notebook are written so noqa is never necessary. If we find a particular type of noqa is often necessary, that suggests that there might be a legitimate case that it shouldn't be considered a flake.

[jlstevens]

Notebooks are scary and confusing enough without magic comments. These comments will scare people who aren't developers.

[jbednar]

'avoid importing something that's not explicitly used, so noqa is never necessary in the first place'

That's what I meant, yes.

[ceball]

Ok, sounds like we're all in agreement, then. Nobody wants to see noqa in notebooks.

I think I've only encountered one time where I've used noqa, and it was to mark an apparently unused import as being necessary for a particular side effect, where I did not understand why it had to be that way, but it was outside my control.

[ceball]

Philipp was right that pyflakes would complain about the change he made to examples/user_guide/Plots_and_Renderers.ipynb:

$ cat test.py
import holoviews as hv
import holoviews.plotting.mpl
renderer = hv.plotting.mpl.MPLRenderer.instance(dpi=120)
$ pyflakes test.py
test.py:2: 'holoviews.plotting.mpl' imported but unused

Is that the best way to do it?

I admit I'm now no longer asking in order to make this notebook clearer than it already is, but because I may need to do something in nbsmoke if we have code in notebooks that pyflakes complains about.

Relevant pyflakes issues: PyCQA/pyflakes#159, PyCQA/pyflakes#248

[philippjfr]

@ceball Tbh, I think we should just replace that with:

renderer = hv.renderer('matplotlib').instance(dpi=120)

and remove the import entirely. Probably needs some other edits to explain that properly and remove the other mention of hv.renderer.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.