Test Registry project

I made the initial doc of this feature for developers:
https://docs.fast.ai/dev/test.html#test-registry

Please let me know if I missed anything.

Thank you.

@Benudek, after having the experience of managing a few of your PR submissions, I felt something wasn’t right and so I checked in with Jeremy to get clarifications about the code style requirements. Jeremy clarified that fastai tries to be as flexible as possible with supporting different coding styles, and so unless it’s listed in the style guide anything goes as long as it’s reasonable and makes common sense. You can see here my attempt to codify what I thought was the style in my post here.

So now I see that several times I was unnecessarily asking you to comply with the style rules that I thought were so, while they weren’t so in reality. I apologize for doing that. I was acting on my own story and not the reality.

Henceforth, I will let the future PR submissions keep whatever style they are in as long as they comply with the actual style guide, which is much less strict than how I like my code to be and then rewrite things if I feel they can be improved in readability and quality.

1 Like

Ahh thanks for the reminder. Totally forgot why the whole point of this feature :man_facepalming:

Couldn’t decide on the UI or what it should look like. Have any thoughts?
Modal seems a bit more clear, but it completely fails if you click the link from a notebook.

Default view:

Expanded card view:

Modal view:

1 Like

Looking good, @ashaw!

My vote is for simplicity, and we can always improve it down the road. so my vote is for whatever works everywhere.

Moreover, while the layer pop-up is neat, it also obstructs the normal page, so I think the simple solution is just perfect as it works everywhere (right?) and it lets the user choose whether to look at the test info or look at other info near by.

Perhaps a collapse button to hide the expansion if it is no longer desired?

1 Like

Anything you suggested Stas was better than what I proposed! Serious, no doubt,no worries at all :slight_smile: thx for the learning curve

1 Like

think its great ! cool. And yes: using real names of instances makes absolutely sense because if names of classes changes, it does not break. It just slipped my mind really. I linked this section also into the testing wiki section here: Improving/Expanding Functional Tests

I was thinking in the general style guide we could add suggestions like

  • try to use join loop constructions where possible e.g. to generate strings:print(’\n’.join(str(missing_this_test) for missing_this_test in TestAPIRegistry.missing_this_tests))
  • Use f notation instead of + to build strings: missing_this_test = f"file: {filename} / test: {test_name}"

Was looking for a good place to add such small suggestions - maybe here? Dev Projects Index

1 Like

Heh, except there is an even simpler way, have a look at the last incarnation. you just print the variable as *var, no need even for join there.

Anywhere you feel is a good place to compile such suggestions is good. except not in project index - its purpose is just that - index.

@ashaw, would it be possible to change the current click on [test] UI functionality?

Currently we have:

<a class="source_link" data-toggle="collapse" data-target="#DataBunch-pytest" style="float:right; padding-right:10px">[test]</a>

So when I click on [test] the browser first focuses on the anchor, shifting the content up, then it opens the test entry which is quite further below. This is a very spastic behavior and I find it extremely difficult to use.

For example:

  1. click on: https://docs.fast.ai/basic_train.html#mixup
  2. click on [test] next to the create_opt or dl entry below

and you will see what I mean. I had no idea what happened. I had to look for the test entry, which was very complex and intuitive. Let me know if you see what I mean.

Perhaps it should:

  1. not refocus on that anchor and just uncollapse the hidden text - i.e. clicking on [test] should not trigger browser location update
  2. move the [test] anchor where it actually gets opened and not at a distance (i.e. not next to [source], but after or next to the docstring?

Perhaps the collapsed section should go before the docstring? See the snapshot how test breaks the flow of the doc at the moment.

but I realize it probably would make things much complex if we want to inject the collapsed show_test output between the function definition and its docstring. Perhaps doing it before the function definition? not great either.

What do we do in case where test_foo calls a helper function (e.g. wrapper) that calls this_tests? Currently it blows up badly.

For example tests/test_basic_train.py:

def test_memory(data):
    subtest_destroy_mem(data)
[...]    
def subtest_destroy_mem(data):
    this_tests(learn.destroy)

Fails in:

def get_parent_func(lineno, lines, ignore_missing=False):
        "Find any lines where `elt` is called and return the parent test function"
        for idx,l in enumerate(reversed(lines[:lineno])):
            if re.match(f'\s*def test', l):  return (lineno - idx), l # 1 based index for github
            if re.match(f'\w+', l):  break # top level indent - out of function scope
        if ignore_missing: return None
>       raise LookupError('Could not find parent function for line:', lineno, lines[:lineno])
E       LookupError: ('Could not find parent function for line:', 159, ['"""\n', 'module: basic 
[it dumps the whole test module here]

Ideas?

Should we just clean up that Exception not to dump the whole file and give a better error message that this_testscan be called only directly fromtest_foo` functions? This makes it somewhat harder to pass the live api, since the object might be only visible inside the helper function.

Or support nested functions so that inspect could reach up say a frame or two and see if the parent or grandparent are test_foo function?

Had to tweak the logic even more to support this_tests checks in skipped tests via decorator and dynamically from the test, but I think all bases have been covered.

I also introduced a special case of this_tests('skip') for situations where it’s not fastai API or it’s an accessor, which we currently don’t support.

All in all a bit of perl and some manual tweaking and we have now all existing tests covered: 233 test_this calls, 38 of which are skipped.

@ashaw, the docs api only contains callables, correct? i.e. there is no entry for things like Learner.data.

and https://docs.fast.ai/ now contains [test] for each test that we have. Please browse around and see if you detect any problems.

2 Likes

@ashaw, a few small tuneups:

  1. In the case of found tests, we probably can drop the “This tests:” line from the test box:
Tests found for `accuracy` :

This tests:

* `pytest -sv tests/test_metrics.py::test_accuracy` [[source]](https://github.com/fastai/fastai/blob/master/tests/test_metrics.py#L42)
* `pytest -sv tests/test_vision_train.py::test_accuracy`

it feels redundant.

Instead we should help users find out how to run those tests, so should probably add after the list of tests:

To run tests please refer to this [guide](https://docs.fast.ai/dev/test.html#quick-guide).
  1. In the case of:
No tests found for mean_squared_error

Should we add 2 links to https://docs.fast.ai/dev/test.html
Improving/Expanding Functional Tests inviting contributions?

So perhaps it’d look like:

No tests found for mean_squared_error. To contribute a test please refer to [this guide](https://docs.fast.ai/dev/test.html) and [this discussion](https://forums.fast.ai/t/improving-expanding-functional-tests/32929).

Thank you!

2 Likes

So registering with this_tests is done? Otherwise wanted to put that on my agenda to work on it when I find some time (which might be a little slow at times with other obligations)

Think inviting tests would be great, one of the main (hopefully successful) motivations for this work here.

Just wondering if the main how to of the forum thread should go into the official doc and the forum is just then pure coordinating.

Hope it makes sense

Great suggestions! Just added all these fixes on master

I probably won’t get around to the jumping collapsible UI until Monday though

1 Like

Can you remind me what Learner.data refers to again?

If you are talking about modules/classes, it should work already:
show_test(Learner)
show_test(vision.data)

I guess that wasn’t a good example, please try:

def test_foo(learn):
    learn = fake_learner()
    this_tests(learn.loss_func)

loss_func is an example of a property attribute.

Great suggestions! Just added all these fixes on master

That looks good. updated just one doc file with your changes, e.g. https://docs.fast.ai/basic_train.html#Learner.create_opt

I probably won’t get around to the jumping collapsible UI until Monday though

Sounds good, @ashaw

I’m not sure I like:

this_tests('skip')

It could be too confusing in the test, where skip has a special meaning. Probably, should pick another short name instead, but I can’t think of a good word so that it communicates a complex thing.

Basically we are trying to say one of the two:

  1. this tests something that is not a fastai API
  2. this tests a property and it is not callable

Two very unrelated situations to be expressed in one intuitive word.

this_tests('none') is the only thing that comes to mind, but that’s not true either because the test is testing something.

Looking through synonyms: omit looks intuitive, and probably won’t be used as a real function anywhere.

update: ok, I think I sorted it out - I changed it to this_tests('na') na = not applicable, not available, etc. - covers all bases and is short.

Yes, all done. Just needing to tune up the UI, then we can announce it.

There are a few edge cases in logic when tests get skipped, but we will sort it out in time.

1 Like

Great work, thx for you leading and executing @stas

1 Like