Include tests in source distribution

[nelson-liu]

Last build-related PR, sorry! It's quite surprising to me that setup.py has commands for running the tests, but the tests aren't actually included in the source distribution that is uploaded to PyPI.

Let me know if there's a good reason for not distributing the tests with the tarball source distribution --- afaik, distutils includes directories named test by default, which seems like a pretty sane decision to me.

[DeNeutoy]

A couple of issues here:
This test downloads all the actual models, and checks that their predictions don't change - can you exclude it?

The test here requires a kernel to be built first, so we should also probably exclude it.

Also, can you install this and check that all the tests do pass?

[nelson-liu]

This test downloads all the actual models, and checks that their predictions don't change - can you exclude it?

Not sure why this is a problem --- does it just take too long to run / too much disk? I feel like including the whole test suite in a source distribution makes sense, and it feels a bit odd to selectively remove things from a tarball that is supposed to encompass the source + everything needed to run it and ensure it works.

The test here requires a kernel to be built first, so we should also probably exclude it.

I think this test will automatically skip if there isn't a CUDA device. I've run it on a GPU machine and never done anything special with the extensions (i.e., explicitly compile it) and it seems to work ok? Also in favor of leaving it for the reasons above (completeness). Arguably, it's valuable information for the user if they run this test and see that it fails.

[DeNeutoy]

Ah the GPU one is pre-compiled, so yes it would work on a GPU machine with the right compute requirements, so that's actually fine. We should definitely remove the sniff_test test, because it tests for correctness of the models, which is not what a user of allennlp as a library needs to be testing. Downloading all the models will take a while and is very surprising behaviour for just running a test suite.

[nelson-liu]

I think this is useful for more than just users of allennlp, though --- I'm personally interested in using the pypi tarball to package allennlp for other distribution channels (particularly conda), and it's nice to be able to run the entire test suite as part of the build process to ensure that the package is correctly built.

[schmmd]

@nelson-liu thanks for all the build-related PRs! Can you give me a code example for what the change in this PR enables? I don't know too much about setup.py / pip.

[matt-gardner]

@schmmd this would fix #856, by just including the tests in the pip install. I'm not sure how the tests would be invoked, though; @nelson-liu will have to tell us that.

[nelson-liu]

right, so this lets you do python setup.py test on the pypi tarball. This PR is a prerequisite to fixing #856, but unfortunately doesn't do it directly since pip doesn't yet have a good way of interfacing with the tests.

I like @joelgrus 's suggestion of making a new command that just runs all the tests.

[nelson-liu]

btw, I more or less have code for #856 already, but the annoying thing is that all the tests have paths that are hardcoded assuming that you're running py.test from the project root --- this will almost certainly not be true if a pip user is testing. The cleanest solution seems to be to replace all the filepaths used in the tests to be relative, but that also seems like a lot of work :)

any other ideas before I take the plunge?

[joelgrus]

can the code do an os.chdir to the right directory, run the tests, and then change directories back? I don't know if that would work

[matt-gardner]

I'm not sure it's worth taking the plunge, and I'll defer to others about other potential solutions here, but if you do take the plunge, would switching to using os.path.join() help anything with supporting windows? If we're modifying all of those lines anyway...

[nelson-liu]

@joelgrus awesome, that works! I'll push this up for now and you all can decide whether it's an acceptable solution and/or whether the move should be made to switching to paths in tests from something like 'tests/fixtures.../' to os.path.join(AllenNlpTestCase.PROJECT_ROOT, 'tests/fixtures.../')...

[joelgrus]

btw, os.path.join is old and uncool, if we go that route we should do

(in test_case.py)

import pathlib
PROJECT_ROOT = pathlib.Path(...)

and then use it like

PROJECT_ROOT / 'tests/fixtures/...'

[matt-gardner]

@DeNeutoy doesn't like the fancy syntax =). Too much like scala? I didn't know you could do that, though, that's good to know. But I was more worried about the / in tests/fixtures/... - are those a problem for windows, or is python smart enough to handle those things? (The fact that I don't even know the answer to this question should make it pretty clear why windows isn't supported...)

[nelson-liu]

But I was more worried about the / in tests/fixtures/... - are those a problem for windows, or is python smart enough to handle those things?

I think you can use os.path.normpath to convert / to \ on Windows, but you're right that it's an issue as is. Maybe pathlib can also handle this?

[nelson-liu]

pinging back about this, is there anything else that should be discussed?

[matt-gardner]

I think the only outstanding question for this PR is whether to add an exclude line for the sniff tests.

[nelson-liu]

I see. I think it's worth including for completeness of the source distribution + hopefully #1213 solves the problem of users being surprised that the tests are downloading large files and taking awhile.

Besides, users who pip install allennlp just to use allennlp serve would probably like to test that serving with the default models works properly, so I don't think the sniff_tests are totally useless to users.

[matt-gardner]

Deferring to @DeNeutoy, who's thought about this more than me.