Test Registry project

ashaw · February 17, 2019, 9:00pm

Oh yeah I had to add an import inspect at the top for one of the tests. So not completely from notebook

Benudek · February 17, 2019, 10:45pm

ok, I didnt do that test to manualy interrupt indeed.

For this specific part I can look at yield but probably we should just say when ever there is an empty dictionary, we just don’t write the file and we are good

I will look at it soon, wanted to check your comment on this here too: https://docs.python.org/3/library/collections.html#collections.defaultdict

That might be a little tricky: because how would I know what is an existing API - inspect code? I was rather thinking later when calling up the docs, one needs to handle that. @ashaw Essentially I would think there is a dead entry in the json db and folks just never call doc_test for such an API, that doesn’t exist.

It might look to me a little like we trying coming with waterproof code for a situation that rarely happens with little consequences, E.g. someone interrupts the testrun, personally would not be too surprised then some test functions might not work properly and just rerun things. I get one has to write code that delivers clean, atomic commits when sending money between bank accounts but for this docs that might be acceptable. 2nd scenario: some coder registered the API incorrect, that means there is dead entry in the json db and the doctest wont work with that test. Or do I overlook some more serious consequences here?

stas · February 18, 2019, 5:47pm

Sorry, no cookie. It will be {} only if none of this_tests calls were made, which in is the case now when only one test module has those calls. Try to think globally and not circumstantially here.

Remember that ideally you should find a way to mark ‘pytest’ completed 100% and w/o errors, and use that. If such is possible and if not invent one. I trust you can, @Benudek

You check that this symbol exists. https://docs.python.org/3/library/functions.html#id

python -c 'import fastai; print(id(fastai));'
140380881709000

You will need try/except for when it’s not a python object.

Also numbers will always id as true:

python -c 'import fastai; print(id(123));'

so you should exclude this case too. There are might be other cases. I’m sure someone already wrote code on SO that covers them all.

update:, no this won’t work since a random string will have id too. Need to research this some more.

But actually, I think you just need to try/except that block where you try to resolve that argument - since it’ll fail if it’s a random string as it does now.

@ashaw, the doc_gen probably already contains some code that can check whether a random string can be mapped to a fastai API, no? is_api("this should assert") vs is_api("fastai.Learner.foo")

Benudek · February 18, 2019, 8:52pm

@stas its all possible likely (e.g. coming up with a way to mark pyest completed) - but do we really need it and do we use our workforce here well?

If a test fails, we should still register it imho. In any case, it should be fixed soon
I submitted a small PR to write the dictionary only if it is non empty. Under normal circumstances that shouldn’t happen, but you reported you had an empty {} - I could not reproduce that locally with interrupting make test, but hope this check (its anyways a solid check) would avoid that.
Looked at: https://docs.python.org/3/library/collections.html#collections.defaultdict I understand in doc test we could maybe avoid some lines, but personally to me that looked a little hard to read also. In any case, it wouldn’t impact functionality afaict?
Handling incorrect arguments: So essentially we would block coders from writing wrong code. Is that such a likely situation for registering a test with this_tests and wouldn’t it be a a minor bug in any case?

Am not saying it would be wrong doing all this, it would be perfect! Having said that, maybe we should go for a solid alpha release and discuss this open question to start registering tests with this_tests,

generate the doc and then iterate over the code with improvements?

Just my 2 cents, if I overlook the urgency of these adjustments, am all ears and will see what I can do !

stas · February 18, 2019, 11:22pm

It’s because you can’t see the impact an incomplete test_api_db.json will have on our setup, you feel that some of those things are OK, but they are not. test_api_db.json must never change in the eyes of git unless this_tests was added/removed somewhere in the tests. Moreover, we have a situation where some tests get skipped by default, but then can be run with a specific flag. And as such, the test_api_db.json will always fluctuate unless we set a very specific way when it gets generated.

So, it’s possible that instead of trying to dynamically figure out when to update it and when not to, we introduce a new pytest flag (conftest.py), say -doc, which when set will update test_api_db.json. And just like we update all the docs in a routine fashion, it could be added to that routine - run the tests first, the do global docs update second. And the same for release, the flag will be required to be included in make test-full before making a release.

-doctest is already used by pytest for other things, so maybe pytest -regtestapi?

wrt, improvements, yes, those can wait. I have never insisted that they should be out there for alpha. But that’s if you mean alpha includes one test file. If you start putting those calls in other tests it’d be very helpful to have all the guards in place to avoid needing to hunt down the errors - w/o checking it’s too easy to make a mistake and not know you made it. This is for the same reason I suggested we pass real object methods and not strings. But I guess if those checks do get added later, they will flag those mistakes. Still, that’s a wasted effort. But it’s absolutely your call enhancement-wise.

Benudek · February 19, 2019, 7:49am

So, I understand now where you coming from. If this is a given

then we need to indeed keep the file stable and should e.g. exclude failed tests. I am not sure why exactly this is needed (so you want to always check in test_api_db.json to git because you need it for the online doc, right? But what would be the big issue, if the doc w.g. shows a failed test?) but I am sure then you are correct and we should indeed tackle this.

Now to the way to implement this. The static version you describe could be sufficient but the dynamic part doesnt seem quite that hard (lets be optimistic), so let me just try the following.

It seems we just need to access a test report and inspect it in conftest / stop_doctest_collector. As soon as we find failures, we never write the file - that is what you mean, right? No complications with skipped tests and so on?

stas · February 19, 2019, 8:23am

2 reasons:

it must not be a yo-yo and hurdle for us developers, where this file keeps on changing and we have to commit an ever changing file, once removing bits once re-adding them. (unless new tests are added, then the commit will go with the new test)
it will break a release process which relies on a non-dirty git status

if it’s not under git, then it doesn’t matter. But if you want users to be able to use it, it has to be under git.

Now to the way to implement this. The static version you describe could be sufficient but the dynamic part doesnt seem quite that hard (lets be optimistic), so let me just try the following.

No, let’s change gears and implement a simply pytest flag as I suggested in the last post. Otherwise we will never finish this and there are probably other situations that we haven’t envisioned. With the flag to save this file a maintainer has a full control at when to update this file, which I believe when the main docs are updated.

Benudek · February 19, 2019, 8:45am

Write test json db only if all tests passed & no empty dictionary

I just checked the dynamic part and that function pytest_runtest_makereport just following the example code gets called at the end of individual tests, not after the entire test run.

Not exactly the trigger we want (after all tests ran accessing all results would be ideal) and it seems the name ‘pytest_runtest_makereport’ is needed also to call it. Still, if we just set a flag if things failed we could use that, so placed that into a PR for your review: https://github.com/fastai/fastai/pull/1673

Good you were anticipating that issue, that complication was totally unknown to me!

Benudek · February 19, 2019, 12:48pm

@stas Independently of above PR to adjust the json db only after success: I do agree this better be that semi-manual process you described as we might overlook complications and just wanna finish this.

–> see PR here: https://github.com/fastai/fastai/pull/1673

Generate json db only on full Release:

new pytest flag -regtestapidb:

in conftest.py check with sys.argv
only if this param was used when calling, then we will regenerate test_api_db.json .
File Makefile:
the flag will be required to be included in make test-full before making a release:
test-full: ## run all tests, including slow ones, print summary
pytest --runslow -ra -regtestapidb

Process:

when we update all the docs in a routine fashion, we will regenerate the tests first

Benudek · February 19, 2019, 3:01pm

@stas, can see the value of blocking incorrect usage of this_tests before registering, summarising that below.

But a try except pattern to e.g. not register

this_tests(‘blah’)

seems like a non-pattern: it seems better the developer putting this in simply gets the exception and the PR can never go through rather than hiding the exception.

I think you want to block registering cases like this_test(print) ? in PR: https://github.com/fastai/fastai/pull/1673

this_tests(learn.abc) --> non existing var / method. This will just throw an exception for the developer
this_tests(1234) --> some number, gives an exception - good
this_tests(Learner.fit) --> take the instance version -block? I guess sometimes we call class methods
this_test(print) --> its not a fast.ai function. doctest.full_name_with_qualname gives builtins.print, so I filter this out with a match

Blocking incorrect Arguments in this_tests

Goal:

if a coder adds an incorrect argument, one that is not a fast.ai method e.g. this_tests('blah') we will not inject this string into the test api db
this would be specifically useful if we can dispatch the task as ‘low hanging fuit’ to other coders

Implementation:

try/except the argument where you try to resolve that argument. A fast.ai API should not cause an exception where was a random string will.
```
  python -c 'import fastai; blah;' --> exception
  python -c 'import fastai; learn.fit'
```

Remark: will this work? E.g. if we give incorrect params we also get errors even though the function might be part of fastai?

use inspect python: check if function, right package and so on to determine it is a fast.ai function

stas · February 20, 2019, 5:22am

OK, I did some reorg/cleanup and tried to give more consistent/intuitive names, so the last incarnation is: TestAPIRegistry and the flag is matching --testapireg. So we are creating a Test API registry. Hope that is an intuitive change. Too bad doctest is already used for something very different in the python world.

Note, I moved almost everything out of conftest into doctest.py, and it’s starting to emerge into a normal class, so we might switch to that eventually, instead of using class methods. But it’s fine for now.

I implemented the check for valid fastai API. Your code already had everything that was needed, just needed to add a few exceptions and word them so that it’s quickly easy to see what needs to be done.

I added a test that tests test_this. Please check that all cases were covered.

For your convenience, in conftest.py I left: #individualtests = 0 so you can uncomment it to do quick tests, without needing to finish make test.

I will go over your notes earlier and make a new summary, including new issues:

Some tests will sometimes add the same entries more than once, see this example. So perhaps we should check for already existing entries in our registry? Surely, the test could be modified to hardcode "fastai.basic_train.load_learner" but why put more onus on the developers when we can easily remove dups automatically.

I reviewed your previous comments and unless I missed something I think the only unadressed item is:

I use inspect.currentframe while I seem to remember we want to just say currentframe and import inspect accordingly

but I’m not sure what you mean there.

stas · February 20, 2019, 6:46am

Added support for strings as arguments.

So now you can do:

    # function by reference (and self test)
    this_tests(this_tests)

    import fastai
    # explicit fully qualified function (requires all the sub-modules to be loaded)
    this_tests(fastai.gen_doc.doctest.this_tests)

    # explicit fully qualified function as a string
    this_tests('fastai.gen_doc.doctest.this_tests')

(side effect to fix from the previous comment of mine - this now registers this_test 3 times!)

and these will fail with either ‘not a function’ or ‘not a fastai api’ errors:

    # not a real function
    this_tests('foo bar')

    # not a function as a string that looks like fastai function, but it is not
    this_tests('fastai.gen_doc.doctest.doesntexistreally')

    # not a fastai function
    import numpy as np
    this_tests(np.any)

Benudek · February 20, 2019, 6:49am

I mean

sth like

from inspect import currentframe

so we say currentframe instead of inspect.currentframe. Think somewhere you mentioned we want to avoid this package notation ?

Should check and make a small PR

Benudek · February 20, 2019, 6:52am

great stuff, thx @stas for getting this over the finish line.

When we are good to go, we could put up guidelines, how to use this_tests and then can start ?

I was thinking to make a one PR per test_ file so we can commit them independently, if sth is missing. I did se in test_train you added a a Recorder as tested next to fit. So, you suggesting we also add APIs tested that are indirectly tested as used by the main function?

stas · February 20, 2019, 7:02am

No, it was just an example to test multiple args, I removed it. Thank you for remembering that odd case.

Ha, now test_this_tests inserts test_this 5 times (!) - so we should definitely remove dups

We could “test_this” objects too, but that’s if it’ll help the docs. Then yes, why not.

I suggest we wait till @ashaw gets the docgen side fully operational. In case we discover some gotchas we haven’t thought of.

There are now enough to work with to build the docs. So let’s see how that unfolds and then expand into other tests?

Andrew, make test-full will now generate the registry file. Of if you don’t want to wait , comment out #individualtests = 0 in conftest.py and then run:

pytest -sv tests/test_gen_doc_nbtest.py tests/test_train.py tests/test_basic_train.py --testapireg

Let us know if you need anything else to complete the bridge. Thank you.

Benudek · February 20, 2019, 7:06am

ok, afaict no action on me.

Might pick some normal test scripts, when I find the time.

thx Stas, fun initiative!

stas · February 20, 2019, 7:07am

There is one as posted above:

e.g., now test_this_tests test inserts test_this 5 times (!) - so we should definitely remove dups

Benudek · February 20, 2019, 7:08am

ok, let me check this

stas · February 20, 2019, 7:13am

Yes, please.

stas · February 20, 2019, 7:21am

Does pytest have a hook/callback that will be called at the end of each test?

If it does we could check whether each test has this_tests to help new test writing, so that this function won’t be forgotten? And give a warning if a test has no this_tests in it? Otherwise it’s difficult to enforce optional, yet, important things.