Initial go at next version, improve maintainability #19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ding some docstrings, and improving test coverage. Some ipython magic handling has probably changed (should be an improvement/more consistent behavior).
jbednar
reviewed
Jun 10, 2019
There was a problem hiding this comment.
@julia, I'll leave it up to you whether to include the magics processing; if it's not needed for our current websites I'm inclined to omit it due to the various disclaimers it has). Also, verify.py mentions being copied from "datashader's examples/nb", but I can't find any file or directory like that, nor any VerifyNb references, in datashader. Maybe that's long gone?
|
Yes, I'm also very happy for decisions to come from @jsignell. Here's my recollection/opinion for background... We must have removed the "verify nb" code from datashader after the work for this pr was originally done, and hence that comment is out of date (in this pr, verify has just been broken out to its own file; the code is already present in current nbsmoke). I think the original source for datashader's verify was bokeh (I was not the author of any of it, and did not add it to datashader - it's likely I just wanted to remove it from datashader and have it "in one place"...of course in doing that I just ended up adding it in yet another place ;) ). As for the magics processing for flake checking, I'd probably keep it all to start with (assuming it works!). Once there's no more hv opts magic in pyviz notebooks, then I'd happily delete the hv magics file. (Or if already at that stage, it could be deleted now.) The rest of the magics processing contains at least some things that are useful for flake checking notebooks in general. E.g. %time f() gets flake checked as f(). So maybe decide which bits are useful, then aim later to replace (parts of) their implementation with what is available from jupyter/ipython (e.g. surely parsing can come from them...)? |
|
Sounds good. The hv magics should all be gone already, but if we missed some, then not having support for them would be a good way to find any that are left so we can fix them! |
|
Ok, makes sense to remove the hv magics handling in this PR then (I'll do that). |
|
A note about the notebook flake checking in general. To give an idea of the kinds of things it finds, see e.g. If notebook flake checking doesn't produce false positives and is quick to run (I think both should be true...), I'd be in favor of enabling it for projects. As part of this pr I'll run flake checking over all the pyviz projects and report the results back here. |
|
Sounds great, thanks! |
|
I think this is all a good plan and will make it easier to maintain nbsmoke. I just wrote a sphinx time magic stripper the other day... So @ceball if you have the time to take out the holoviews magics stuff that'd be great. Let me know when you are done. |
|
Re Python 2, I don't think it is very important at all to have nbsmoke work for it. |
Right; we can just run smoke tests under py3; no problem. |
…gics). Also added test that cell magic skipping is working, since such a test was missing.
|
I have not yet removed hv magics handling, but I've run this PR's nbsmoke over all pyviz projects (I think I got them all!). I'll re-run after removing hv magics handling, but a few questions have come up:
Output follows... (Note: the timings are unlikely to be realistic for your hardware; they were run in the background on a win/32bit/2GB/1GHz atom laptop, which was primarily showing kids tv ;) pyvizgeoviewsholoviewsdatashaderpanelhvplotcolorcetparamexamplesearthmlearthsim |
Thanks for this! I personally really hate display() being in the namespace automatically, and I think people should import it. So I think nbsmoke should keep complaining about it, particularly so that notebook code can be used in non-notebook contexts (e.g.
We agreed not to use them, but haven't removed all examples of them. We'll need to make a PR in each case.
Good question. If it's straightforward to add a user-extensible whitelist of unused (sideffecty) imports (presumably as globs
It seems like using
For an exercise, sure; that wording sounds good. In general, I think we should try not to have #noqa whenever possible, but I don't see how we will always avoid it.
Any idea if these issues are real ("invalid escape sequence")? |
|
Chris, thanks so much for running it on all those projects! It is abundantly clear which ones have linting enabled in CI and which don't... For Yep I agree. I actually think #noqa with an explanation for hvplot imports isn't such a terrible idea. It makes it clear what is going on and these are all examples so they should be explicit. I've been seeing the escape sequence warning cropping up in various places, but hadn't looked into it yet. |
|
About the invalid escape sequence warnings: their appearance is probably down to whatever warning filter is used by pytest, but it's not an nbsmoke or a pytest thing:
|
|
Ok, sounds like those warnings are real and we should be adding escape characters to the strings in those notebooks. Thanks. I'm happy with either the whitelist or the #noqa approach for hvplot; I'll leave that up to you two. |
|
I've fixed the flake checking under python 2 for the benefit of charities etc that can't afford to upgrade from python 2. Still got the appveyor/windows tests to fix (I'm using windows locally, so surprised they don't just pass..). Also still got the conda tests to fix (I can't run conda locally - laptop too slow :( - but I should be able to figure it out from the travis logs later). |
…ests all fail. See how py2 is getting on now...
…ally sure which packages I should be installing to provide the imports required by nbsmoke).
* Support for flakes to ignore. * Support for magics blacklist. * Separated cell and line magics. * Increased number of builtin magics that will be ignored. * Support for capture, script magics (mainly as demo).
ceball
changed the title
|
I guess the appveyor tests have been lost in one of the pyviz renames? :( Although I am developing on win, the appveyor tests were failing when I last worked on this PR. |
|
Running again over some of our projects (nbsmoke's been run before on some of them in the past, but maybe not for a bit recently)... geoviewsholoviewsdatashaderhvplotWithout ignoring any flakes: Ignore deliberate side effect imports: See holoviz/hvplot#298 for output.
holovizpanelSee holoviz/panel#631 for output
examples / pyviz etcTODO |
Hmm yeah I can look into that. I'm a little surprised they are failing since you are the only one who's been making changes... |
|
I remember I was also surprised to find appveyor was failing for this PR since I was developing on windows, but it was definitely failing. I recall now I could not immediately figure out why it was failing. That's part of the reason this PR stalled before. Maybe I should get it in before you restore appveyor? :) |
|
That is fine by me :) We don't need it to run on windows. |
ceball
changed the title
|
@jsignell I'm sure I'd like to improve the code, but it would also be good to start trying to use this version before another couple of years go by ;) So I'll happily fix or clarify stuff in response to any comments you make, but otherwise I'm done with this PR. |
jsignell
reviewed
Sep 11, 2019
nbsmoke/lint/__init__.py
Outdated
| with io.open(self.name,encoding='utf8') as nbfile: | ||
| nb = nbformat.read(nbfile, as_version=4) | ||
|
|
||
| magics_thing = magics.Thing( |
There was a problem hiding this comment.
I think proper names would help in general with debugging, but it seems like you are moving towards that :)
There was a problem hiding this comment.
Yeah, I don't know how to name it because I don't know what it is. The problem is that it's all grown organically as there was no plan about what I wanted out of nbsmoke in the first place, I didn't know pytest, etc, so it was just a case of typing until it worked :)
# TODO: suddenly had to make some fns into a class to support blacklists; should rework.
class Thing(object):
I've renamed magics thing to magics processor, but that's really just cosmetic.
It can definitely be reorganized. If we're still using it in a year's time, I'll be prepared to fix it ;)
nbsmoke/lint/__init__.py
Outdated
| _user_flakes_to_ignore = _user_flakes_to_ignore.splitlines() | ||
| for pattern in set(flakes_to_ignore) | set(_user_flakes_to_ignore): | ||
| flake_result['messages'] = [msg for msg in flake_result['messages'] if not re.search(pattern, msg)] | ||
| ### |
There was a problem hiding this comment.
Is there meant to be a comment here?
There was a problem hiding this comment.
It marks the end of a comment block (good old commented chunk instead of a function...). Have clarified.
| debug=self.config.option.nbsmoke_lint_debug | ||
| filenames = [] | ||
|
|
||
| self._write_debug_file(debug, ipy, self.name, "pre", filenames) |
There was a problem hiding this comment.
It might make sense to have this be a context manager rather than calling it all over the place.
There was a problem hiding this comment.
True. It's just twice though, and didn't change in this PR (you're seeing code that's moved from the original single file into its own one, I think).
nbsmoke/lint/__init__.py
Outdated
| raise NBLintError(msg) | ||
|
|
||
| @staticmethod | ||
| def _write_debug_file(debug, py, name, what, filenames): |
nbsmoke/lint/_pyflakes.py
Outdated
|
|
||
| * return more info for use by caller | ||
|
|
||
| * support "noqa" in ipynb (of questionable value...) |
There was a problem hiding this comment.
maybe add a note about hvplot?
There was a problem hiding this comment.
noqa support and this note predates the current PR, and was originally added (a couple of years ago now?) for a project that had a module imported for its side effects (unrelated to hvplot).
I've updated and clarified.
(What's added in this PR is support for ignoring flakes by regex, without requiring noqa at the flake itself.)
| @@ -40,15 +40,26 @@ def read(fname): | |||
|
|
|||
| install_requires=[ | |||
| 'pytest >=3.1.1', | |||
| ########## lint stuff | |||
| 'pyflakes', | |||
There was a problem hiding this comment.
Is this really needed - aren't we vendoring it in at this point?
There was a problem hiding this comment.
Yes, it's needed - there's more to pyflakes than the _pyflakes file you see in nbsmoke...like all the rules etc! :)
We only modify one pyflakes file, to return more info from flake checking and to support noqa (which pyflakes doesn't support - we could use another linter that does, but I don't think we even run another linter on any of our code, let alone notebooks...).
|
@jsignell, thanks for taking the time to review! Can't have been pleasant ;) Let me know if I missed anything. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added some basic missing features, and cleaned up some internals.
New features
support for flake messages to ignore (by regex) - fixes Lint checking: Allow user configurable ignoring of flakes #22
support for magics blacklist (i.e. magics that if present cause a flake) - fixes Lint checking: Add user configurable magics blacklist? #21
added support for script & capture cell magics (mainly as a demo) - fixes Lint checking: support more built-in magics #24
increased the number of builtin magics that will be silently ignored (as they don't interact with the python process), e.g. bookmark, edit, who, etc - fixes Lint checking: support more built-in magics #24
lint failures can be set to be warnings only (potentially useful for systems that report warnings)
Internal changes
Make nbsmoke a bit more maintainable by separating out the various parts, adding a few docstrings, and improving test coverage.
Some ipython magic handling has probably changed (should be an improvement/more consistent behavior).
This work is over a year old. I think the need to lint check notebooks that involve a lot of magics has gone away since this work was originally done. However, the changes here still bring some benefits (e.g. the modularization will also allow functionality to be removed easily).
Details:
Separated out parts into modules:
the original/basic functionality (checking notebooks run without errors)
"verifying" (copied from datashader, which was copied from bokeh?)
lint/flake checking (using pyflakes)
magics handling for flakes
Improve lint/magics debugging by storing various intermediate files.
Simplify lint checking of magics, making behavior more consistent.
Replaced custom/hacky magics parsing with stuff from ipython itself (fixes Replace custom magics parsing with something from ipython itself #8)
MAGICS_TO_IGNORE and SIMPLE_MAGICS: separated cell and line magics (internal change to support more magics) - fixes Lint checking: Separate magics skip list into cell and line #23
Improved unit test coverage (think all code is now covered except "verify").
Still to do this PR:
restore py2 support
fix travis
fix appveyorremove hv opts handling (probably will do in future PR)address Lint checking: Separate magics skip list into cell and line #23
Run the lint checking over notebooks of various pyviz/holoviz projects as a sanity check.
Future work:
Figure out how to measure test coverage in a way that doesn't miss out module-level lines (module-level definitions, imports, etc are all missed because apparently they happen before coverage collection begins; I suspect this is something to do with my not knowing how pytest plugins work) Self tests: how to get accurate coverage report? #26
Find datashader/bokeh "verify nb" unit tests and copy them in here? (verify nb stuff is copied from datashader, which appears to be from bokeh originally...I think the original plan was to replace datashader/bokeh "verify nb" with nbsmoke...) nb "verify" functionality is not tested #28