Skip to content

re-write of test flags#232

Merged
jph00 merged 5 commits into
AnswerDotAI:masterfrom
pete88b:test-flag-rewrite
Sep 4, 2020
Merged

re-write of test flags#232
jph00 merged 5 commits into
AnswerDotAI:masterfrom
pete88b:test-flag-rewrite

Conversation

@pete88b
Copy link
Copy Markdown

@pete88b pete88b commented Sep 3, 2020

Magics flags for tests

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@jph00
Copy link
Copy Markdown
Contributor

jph00 commented Sep 3, 2020

Thanks much better! :D Let me know when you've tested on colab, and I'll merge this and see how it goes.

@pete88b
Copy link
Copy Markdown
Author

pete88b commented Sep 3, 2020

hi @jph00 bit slow going (o: but this all works on colab.

Hope you're ok with the extra changes to 04_test.ipynb and 09_nbdev_callback_test.ipynb - in its own cell, import nbdev_callbacks might fail or might load the wrong module (depending on sys.path etc), we need:

call_cb('this makes sure nbdev_callbacks is loaded from the right place')
import nbdev_callbacks

@jph00
Copy link
Copy Markdown
Contributor

jph00 commented Sep 3, 2020 via email

@pete88b
Copy link
Copy Markdown
Author

pete88b commented Sep 3, 2020

@jph00 we import nbdev_callbacks so we can fully test call_cb and be able to test that nbdev code (e.g. test_nb) is calling the callbacks properly.

I had said in the docs "If you ever need to import the nbdev_callbacks in your code, please make a call to call_cb first", but you've made me realize we should hide all of these low level details from nbdev users - as they should not ever have to do this (they should just worry about defining the callback handlers that they care about in nbdev_callbacks.py).

I've pushed another change to clean up the imports and hide some more details from the docs.

Hope this makes sense (o:

@jph00
Copy link
Copy Markdown
Contributor

jph00 commented Sep 3, 2020

Ummm.... but now we have non-def/class code running when imported again! :(

Can you explain more about what's going on here? Why do we need this exactly? How long does it take to run?

@pete88b
Copy link
Copy Markdown
Author

pete88b commented Sep 4, 2020

Hi @jph00 nbdev_callbacks.py was added to allow nbdev users to customise the testing and build doc processes.

In response to forum questions, here's a couple of places where callbacks can help and they will simplify creating code coverage reports - I'm hoping we'll find lots of uses for these callbacks that would otherwise have needed code changes to nbdev.

The implementation of call_cb aims to

  • make nbdev_callbacks.py optional (i.e. users can ignore all of this if they don't need to implement any callbacks)
  • make it as simple as possible for nbdev users to implement their callbacks (i.e. require no additional setup steps etc)
  • allow nbdev users to implement their own callback handlers in a regular python module
    • that is not part of the library they are building
      • nbdev_callbacks.py customises nbdev build so it needs to be part of the project (like settings.ini) but not part of the library
    • can be read with a regular python import - but will only ever import the project specific version of nbdev_callbacks.py

Here's an example to show why call_cb has to mess around with sys.path:

I'm working on nbdev and my sys.path contains:

['/home/peter/github/pete88b/fastcore',
 '/home/peter/github/pete88b/nbdev']
  • running import nbdev_callbacks would load nbdev_callbacks.py from fastcore
  • running call_cb modifies sys.path so nbdev_callbacks.py is loaded from nbdev

Note: the only reason we import nbdev_callbacks is to test and document as part of nbdev.

call_cb runs really fast on my machine:

%timeit call_cb('x')
648 ns ± 3.68 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
%timeit notebook2script()
358 ms ± 3.05 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Let me know if a quick call to discus might save you some time (o:

@jph00
Copy link
Copy Markdown
Contributor

jph00 commented Sep 4, 2020 via email

@pete88b
Copy link
Copy Markdown
Author

pete88b commented Sep 4, 2020

@jph00 I think we've got it right now (o:

@jph00 jph00 merged commit 7270c4f into AnswerDotAI:master Sep 4, 2020
@jph00 jph00 added the enhancement New feature or request label Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants