Go for it, as I said several times, please feel free to edit/add what you think will help, and especially those of you who went through a few cycles of submitting PRs and getting feedback and requests for changes are the most qualified to help others with the guidelines. Contributing to contributors is just as important. and thank you.
2 Likes
yes, added that comment on low hanging fruits ;-)
1 Like
Except, what has been added so far is not low-hanging fruit - you linked right back to the main topics that are already linked to, which are big and the user will have no idea which ones are low hanging. so it’s quite misleading.
I think if you want to create a list of low hanging fruit it should be actual actionable items. For example, writing tests in general is not one of them. Finding and fixing typos is.
ok, understand. Corrected - if I see how hanging I would add them to the list then!
I also think this is what we have: https://forums.fast.ai/c/fastai-users/dev-projects for…
So listing all kinds of small things to do would be very useful.
perhaps the small things could go directly into the wiki post, rather than create a thread for each one.
or may be there should be a dedicated thread for small things with the first top post wiki coordinating those.
For example @sgugger has just created learn.purge which resets your GPU memory to almost 100% free (amazing!), so someone could go over the course notebooks and replace gc.collect() calls with learn.purge() and of course, testing that they still work. but may be need to wait till the next release - this was just an example - i.e. watching CHANGES and adjust docs/notebooks to sync with the latest improvements.
2 Likes
To me if the spec is clear and well defined this is a relatively trivial project (the test_this() side of it, and the docs side of it I don’t know - Andrew knows that side), so by now I trust you have all the input you need, and of course feel free to ask questions. Watching someone making small commits as they figure things out is not so fun. That’s why I suggested that I will be happy to look at the code when it’s more or less done, via PR or just name of your branch that you are working in.
Trust me, you won’t want to watch me code in real time, trying all kinds of things, most failing, so it’s nothing personal.
PRs are for ready things with potentially some fixes. They are not a development tool.
The dev way is to create a branch that several committers have access to and work on it together. No need for github’s pretty, but often not so useful interface.
1 Like
ok @init_27 , then we should incorparte these guidelines in the how to contribute I think ! @PierreO
1 Like
[Suggestion]
We could have some low-hanging fruits. For ex: A few “open issues” where new contributors can get started (beginner-friendly issues).
I found it nerve-shaking to be able to even think of being able to submit a PR at first but these simple open issues might help many contributors to find the momentum to get started.
Source: OpenMined community which follows a similar approach to get new contributors get started.
good, some (not all!) of the test scripts are low hanging fruit! They are a good way to get into the code base
1 Like
Thanks Stas, glad you like it ! Hopefully the post will serve its purpose and more people will start to contribute 
Would the FAQ seems like a good place to put these ?
2 Likes
Another thing that might be added is a few sections where we can have more contributions to the docs.
(Adding more examples/explainations, etc)
FYI, I split the doc_test project into its own thread as it completely dominated this topic with 90 posts and growing. and we have other testing-related things to discuss here.
1 Like
Here are some notes that we might need later.
If we want global learn_vision, learn_text, etc. objects, we can pre-create them from conftest.py:
from fastai.vision import *
@pytest.fixture(scope="session", autouse=True)
def learn_vision():
path = untar_data(URLs.MNIST_TINY)
data = ImageDataBunch.from_folder(path, ds_tfms=(rand_pad(2, 28), []), num_workers=2)
data.normalize()
learn = Learner(data, simple_cnn((3,16,16,16,2), bn=True), metrics=[accuracy, error_rate])
learn.fit_one_cycle(3)
return learn
now inside let’s say tests/test_vision_train.py I can access this via fixture:
def test_accuracy(learn_vision):
assert accuracy(*learn_vision.get_preds()) > 0.9
If we use:
@pytest.fixture(scope="session", autouse=True)
all global objects will be pre-created no matter whether the tests need them or not, so we probably don’t want autouse=True. w/o it these will be created on demand.
There is a cosmetic issue with having learn_vision, learn_text, since now we either have to spell out:
def test_accuracy(learn_vision):
assert accuracy(*learn_vision.get_preds()) > 0.9
or rename:
def test_accuracy(learn_vision):
learn = learn_vision
assert accuracy(*learn.get_preds()) > 0.9
both aren’t very great…
We want to be able to copy-n-paste quickly and ideally it should always be learn.foo, especially since there are many calls usually.
Unrelated, with this and even with test module scoped variables (e.g. learn) that we currently use there is an issue that tests can and will modify the object and the next test doesn’t get the same object but a modified one, so this can be very bad for tests, as it can mask serious issues.
Instead we need to have some kind of clone() method which will give each test a clean copy of the object.
Writing tests that exercise APIs that have a large impact on gpu RAM (e.g., purge, load, destroy) proved to be very problematic, since the memory utilization patterns seem to vary greatly depending on:
- what has been run a second earlier to this test, so changing the order of test execution changes the numbers
- gpu card model
So at the moment I have to put all gpu tests in a single test, which could make troubleshooting somewhat difficult.
But the biggest problem is that the test assert numbers are now currently tied to the gpu card model I use and can’t be shared and tested by others unless they have the same model. Sure, we could prepare a few sets of these numbers based on common card models, but it’d be very difficult to maintain these sets as tests and the code base evolve.
I already use a small tolerance in the checks (i.e. not checking exact numbers), but making it large would defeat the purpose of these tests - since we want to make sure that these special methods either actually reclaim memory or they don’t leak memory. e.g. until recently save/load sequence actually wasted more memory than if you weren’t to call these, but fixed now. So we absolutely need to have tests that check for possible regressions and leaks.
And it wold have been easier to run a large tolerance of say 100MB if the tests were to load huge models and data, but it’s not possible in the testing environment since it’d slow things down a lot.
If you would like to see what I have been doing so far, skip to GPUMemTrace calls in https://github.com/fastai/fastai/blob/master/tests/test_basic_train.py
You can unskip the first part of test_memory to run these (unless you have the same card as I then it’ll just run).
You can uncomment:
#check_mem_expected = report_mem_real
earlier in the test module, to print the memory usage instead of asserting on it.
If you have some good ideas on how to handle this problem, I’m all ears.
Until then I will be testing it only on my machine.
p.s. I suppose perhaps the solution is to use large models and data sets, so that the memory usage numbers will become much larger and thus it’d be possible to greatly increase absolute tolerance numbers without defeating the purpose of these tests and thus support a wider range (all?) card models. And make those into slow tests that will be run only occasionally, and certainly before a new release.
1 Like
@stas I cant say I had the time to go into all details of the perf & load tests you do (and if I had, it would take me a moment to understand), but splitting of normal, fast running and always running tests from such larger tests, maybe with larger models it imhe totally the way to go. So, one has two sets of tests - maybe there is a flag or alike. One could also use the fake class maybe to have either small or larger models based on such tests and then of course some test scripts for per & load would simply run only before releases.
Having said that, such tests add really a lot of value (compared to the sometimes trivial assert tests). Great you are helping out. If I had more time, after the current doc test would love to help but can so far only give (hopefully) these ‘smart comments’ here 
So my 2 cents summarised:
- maybe one just has to agree on one reference environment with Jeremy and Sylvain. So you say: we guarantee / run load & perf tests on a typical, standard cloud like Google or Azure. Other setups might deviate but we tested on a standard setup and people can use these tests for their specific settings (this could be an addition to the doc then eventually)
- I do wonder if we might want to separate these complex tests with a switch (so they run only before releases) and maybe even separate out the code base or use naming conventions like test_perfload_[APIXYZ] For example, I could imagine in the doc_test project it would be great one can identify the perf load tests based on the name and @ashaw could potentially dump them later on in a separate section Perf & load tests.
This is of course completely fantastic and complex work you are doing here! 
1 Like
@stas I am afraid I will not get to this task with the attention and effort it deserves and took out my name here: Dev Projects Index
Hope can help finishing and handing over the doc_test project, which takes more effort than expected.
I might find time to show this test stuff at my local meetup and maybe some folks then submit PRs. Hopefully also the doc_test project will motivate others