Fix test-install by moving tests into module #1232

nelson-liu · 2018-05-17T01:26:50Z

Goals of this PR:

1. Prevent tests from being installed as a module in python setup.py . Currently, pip copies them to site-packages, which is definitely not intended behavior.
2. installation method-invariant way to load and run tests, especially those that depend on files outside of the module (e.g. scripts or tutorials). The issue is that files can be in different places depending on if allennlp is installed with pip or setup.py install.

For 2: an easy solution might be to, when running the test-install command, simply download and cache a copy of the allennlp source for the appropriate version at ~/.allennlp . Note that this wouldn't work for non-release versions, but I think that's a good thing --- if you're working on master or developing features, you shouldn't be using test-install anyway.

Thoughts? I'm not sure how i feel about the above solution, so feedback would be great before i write the code.

Edit: another solution is to just move the tests into the allennlp module (this is a fairly standard way of structuring projects anyway). The downside to this is that it still doesn't enable using allennlp test-install to run tests in tutorials, as well as the tests that depend on files outside the module (e.g. SRL test that compares output of evaluator with a perl script). But maybe we shouldn't be running those tests anyway in test-install? or we can move the perl script into the tests and then just not run the notebook tests, since we'll be rewriting them anyway.

This solution is rather time-intensive, though, since it involves fixing all of the paths that are hardcoded into the tests. Should not be too hard to write a script to go through and change the paths, minimizing human labor to just a check up...

edit: fixes #1228

nelson-liu · 2018-05-17T03:34:32Z

Another thing we could do is move the tests within the allennlp module...I know of at least a few others packages that do this (e.g., scikit-learn and pandas. numpy and scipy also put tests within each module). Maybe this solution is a bit drastic? I don't think too much depends on where the tests are actually located (maybe some of the CI scripts? But py.test in the root should continue to work)

nevermind this still doesn't solve the fundamental issue of having no way to reliably access ./scripts and ./tutorials in the tests unless we download the whole source. But do we even want to do that? 🤔

nelson-liu · 2018-05-18T17:45:15Z

I went ahead and made the change, was a lot less work than I anticipated. even if we don't want to move tests within the module, a lot of these commits can be cherry-picked for an eventual windows-compatibility PR.

maybe best to look at this after EMNLP...The diff looks like there are a lot of changes, but 1.9K of the added lines are from adding srl-eval.pl to the allennlp/tools directory.

schmmd · 2018-05-21T16:54:23Z

@nelson-liu what's the difference between tools and scripts?

nelson-liu · 2018-05-21T16:57:42Z

scripts are standalone, one-off entities, while tools are core to some other component of the package (e.g. evalb is essential to the constituency parser metric, and the srl perl eval script is used in the tests). In general, if some outside tool/script is necessary to make a component of the library function properly, it should go into tools.
since scripts are standalone, they go outside of the package. Tools are explicitly called by things inside the package, so they should go inside the package as well --- this lets the package access them after being pip-installed, which seems important.

I named them differently to make it easier to distinguish. If you have better names for them, I'd be happy to change! I agree that it can be confusing...

nelson-liu · 2018-05-22T19:44:15Z

(let me know if there's anything I can do to make this easier to review, as well).

schmmd · 2018-05-22T21:44:21Z

@nelson-liu I think the names are fine. A brief (1 sentence) README.md in each folder would be great, defining the role of each. Overall, this seems like a lot of improvements, but also a lot of changes. As it touches the whole codebase I'm not sure if I'm the best person to code review it.

DeNeutoy · 2018-05-23T17:41:44Z

allennlp/common/file_utils.py

@@ -60,7 +61,7 @@ def filename_to_url(filename: str, cache_dir: str = None) -> Tuple[str, str]:

    return url, etag

-def cached_path(url_or_filename: str, cache_dir: str = None) -> str:


@joelgrus does this Union, and all the str(path) calls in this PR change how you feel about Path?

DeNeutoy · 2018-05-23T17:43:51Z

I'm still unsure whether it's desirable to have users run tests. Can you point me to some other python projects which do this?

nelson-liu · 2018-05-23T17:49:30Z

NumPy / SciPy lets users run their tests through the CLI (https://stackoverflow.com/a/9200923/2544124):

import numpy
numpy.test()

(and accordingly for scipy)

Also theano

schmmd · 2018-05-24T16:22:08Z

For another motivating use case, I'd like to create a Docker image that downstream users can use to run Beaker. It would look like @cristipp 's and would install AllenNLP via pip. Having the ability to test the pip distribution would be invaluable for making sure the image works as expected.

schmmd · 2018-05-25T14:50:43Z

allennlp/tests/commands/test_install_test.py

+from allennlp.commands.test_install import _get_module_root
+
+
+class TestTestInstall(AllenNlpTestCase):


What is the motivation for this test? A brief comment would be helpful. I'm confused because it's a test, in the tests folder, that's checking to see whether the tests folder exists.

It looks like this is a rename rather than a new file. While that doesn't change my confusion, you don't need to address it as part of this PR.

I agree that this is quite confusing. The context is that when a user runs test-install, we have no idea where they're running it from, so we do an os.chdir to the module root in order to get all the paths in the fixtures to resolve properly.

The logic within test-install is pretty hard to test in its entirety (see discussion here), so this test is basically a test for that os.chdir component, checking that we correctly find the path to chdir to.

does that help? I'll add this as a comment too.

Yes that helps!

schmmd

Looks great--just a couple of comments.

Also, srl-eval.pl is now at allennlp/tools/srl-eval.pl and scripts/srl-eval.pl. We should probably just move the latter.

@nelson-liu in the future I think this would have been reviewed more promptly if you had broken it up into smaller PRs. You accomplish a few things in this PR:

Moving tests to allennlp/tests.
Improving the cleanup in wikitables tests.
Changing how we write paths in the tests.

schmmd · 2018-05-25T14:54:55Z

allennlp/tests/training/metrics/wikitables_accuracy_test.py

@@ -29,7 +31,8 @@ def test_accuracy_is_scored_correctly(self):
        logical_form = ('((reverse fb:row.row.year) (fb:row.row.index (max '
                        '((reverse fb:row.row.index) (fb:row.row.league fb:cell.usl_a_league)))))')

-        wikitables_accuracy = WikiTablesAccuracy(table_directory='tests/fixtures/data/wikitables/')
+        print(os.getcwd())


@nelson-liu is this an intentional print?

it's not, thanks for catching that!

schmmd · 2018-05-25T14:55:47Z

mypy.ini

+[mypy]
+
+[mypy-allennlp.tests*]
+ignore_errors = True


Why is this necessary? Did mypy previously not run on our tests?

Yes, mypy was previously not being run on the testing code --- I thought this was intentional?

Seems like you made the right change in this PR then. I'm not sure what we want long term.

@nelson-liu, do the mypy errors on the tests look helpful? I'd guess that we don't care about them, because it's not as important to have clear type annotations in the tests, but if they look useful we could reconsider.

@matt-gardner here are the mypy errors: https://gist.github.com/nelson-liu/f94a96265973a390066f45399718fe5e . I guess they're not entirely useless? Perhaps we should address in another PR though, this one is already huge.

Hmm, yeah, ok, I'll open that as an issue. Thanks for doing all of this!

schmmd · 2018-05-25T14:56:48Z

allennlp/tests/commands/test_install_test.py

+from allennlp.commands.test_install import _get_module_root
+
+
+class TestTestInstall(AllenNlpTestCase):


It looks like this is a rename rather than a new file. While that doesn't change my confusion, you don't need to address it as part of this PR.

nelson-liu · 2018-05-25T15:46:20Z

Thanks for taking a look, I agree that this PR is difficult to review. The main issue is that (2) and (3) are actually not problems unless (1) is done, but in order for (1) to work properly at all (2) and (3) have to be achieved.

I basically just moved the tests, ran them, and started fixing the errors.

schmmd · 2018-05-25T15:48:48Z

@nelson-liu sounds great. If you can move srl-eval.pl instead of creating a new copy, then I'll approve and merge.

nelson-liu · 2018-05-25T15:57:21Z

before you merge, i'd like to run some last sanity-check tests on the code in this branch (building it from github source with pip on a clean environment and seeing if everything works as expected). I've tested it locally, and it seems to work, but i want to verify with a GPU as well. I'll let you know when that's done.

Maybe this is something you might want to add to CI as well? Not sure.

DeNeutoy · 2018-05-25T16:20:35Z

I was semi-serious about not liking the Path thing - I think it will just cause us to have loads of str(path) things everywhere and doesn't really add much. I'd kinda prefer it if this PR switched back to using string paths.... @joelgrus lets argue

DeNeutoy · 2018-05-25T16:22:58Z

Meh maybe i'm just being grumpy and un-modern - I didn't realise it might fix some of the windows compatability issues. If that's the case let's go for it.

matt-gardner · 2018-05-25T16:30:39Z

@DeNeutoy, I agree that the only benefit I see to using the Path stuff is better windows compatibility. Otherwise just using a string path is simpler and easier to read.

nelson-liu · 2018-05-25T20:54:01Z

I'll merge this when CI passes, unless there's anything further to be discussed?

I pip installed allennlp from this branch on an osx machine and a linux gpu machine, and allennlp test-install passed on both hosts.

joelgrus · 2018-05-25T20:57:39Z

to @DeNeutoy 's point, calling str() on pathlib.Path objects is a bad code smell. if the places we need those paths can't accept pathlib.Path object then I would rather just use strings.

(my ideal solution would be that those places accept Path objects tho)

nelson-liu · 2018-05-25T21:04:56Z

thanks all for taking a look

fixes allenai#1228 Goals of this PR: 1. [x] 1. Prevent tests from being installed as a module in `python setup.py` . Currently, pip copies them to `site-packages`, which is definitely not intended behavior. 2. [x] 2. installation method-invariant way to load and run tests, especially those that depend on files outside of the module (e.g. `scripts` or `tutorials`). The issue is that files can be in different places depending on if allennlp is installed with `pip` or `setup.py install`. This is done by moving the tests within the module and using os.chdir to make the paths in the fixtures properly resolve.

nelson-liu mentioned this pull request May 17, 2018

running allennlp test-install after installing with pip leads to many errors #1228

Closed

nelson-liu force-pushed the rework_test_install branch from c90a4e8 to 68483cd Compare May 18, 2018 04:04

nelson-liu changed the title ~~[WIP] reworking test-install command~~ [WIP] Fix test-install by moving tests into module May 18, 2018

nelson-liu changed the title ~~[WIP] Fix test-install by moving tests into module~~ Fix test-install by moving tests into module May 18, 2018

nelson-liu force-pushed the rework_test_install branch from 61dc8fd to 4900696 Compare May 18, 2018 22:28

nelson-liu force-pushed the rework_test_install branch from e4d9622 to 80219c7 Compare May 22, 2018 08:40

DeNeutoy reviewed May 23, 2018

View reviewed changes

nelson-liu force-pushed the rework_test_install branch from 721626f to a1c903c Compare May 23, 2018 23:22

nelson-liu force-pushed the rework_test_install branch from a1c903c to aa62a9e Compare May 24, 2018 17:29

schmmd mentioned this pull request May 24, 2018

Release v0.4.3 #1273

Merged

nelson-liu added 11 commits May 24, 2018 14:27

Prevent tests from being installed as module by setup.py

cf8caa5

Bulk move tests

95784af

Add PROJECT_ROOT, TESTS_ROOT, FIXTURES_ROOT

369b044

Fix fine_tune_test

c79d3c8

Fix dry run tests

26def09

Fix broken import in elmo_test

2d81aae

Fix elmo tests

380f947

Fix evaluate test

a7955ef

Fix imports in adaptive/bucket/epoch_tracking_bucket iterator tests

ac949e7

Move ROOT variables out of setUp

7917d4a

Make cached_path work with Path

fa1adf6

nelson-liu added 3 commits May 24, 2018 14:27

Further cleanup of files leftover by SEMPRE

0d4d000

Remove accidentally committed evalb binary

309cf5f

Wildcard exclude custom_extensions from check_docs.py

5cddce8

nelson-liu force-pushed the rework_test_install branch from aa62a9e to 5cddce8 Compare May 24, 2018 21:27

nelson-liu added 2 commits May 24, 2018 18:08

Merge branch 'master' into rework_test_install

92f9d82

Merge branch 'master' into rework_test_install

00c166c

schmmd reviewed May 25, 2018

View reviewed changes

schmmd self-assigned this May 25, 2018

Remove accidental print in wikitables_accuracy_test

e01ba27

nelson-liu and others added 2 commits May 25, 2018 08:54

Remove srl-eval.pl from scripts

6324d62

Merge branch 'master' into rework_test_install

852afb1

schmmd approved these changes May 25, 2018

View reviewed changes

nelson-liu and others added 2 commits May 25, 2018 12:47

Merge branch 'master' into rework_test_install

65f51a3

Add comment explaining test_install_test

d478ccd

matt-gardner mentioned this pull request May 25, 2018

Enable mypy on tests #1287

Closed

nelson-liu merged commit 59a0e5a into allenai:master May 25, 2018

nelson-liu mentioned this pull request May 25, 2018

Add instructions for getting AllenNLP to run on Windows #612

Open

matt-gardner mentioned this pull request May 25, 2018

Change functions that have filenames as arguments to use Path objects instead of strings #1288

Closed

schmmd mentioned this pull request May 31, 2018

Simple server is broken #1316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test-install by moving tests into module #1232

Fix test-install by moving tests into module #1232

nelson-liu commented May 17, 2018 •

edited

Loading

nelson-liu commented May 17, 2018 •

edited

Loading

nelson-liu commented May 18, 2018 •

edited

Loading

schmmd commented May 21, 2018

nelson-liu commented May 21, 2018 •

edited

Loading

nelson-liu commented May 22, 2018

schmmd commented May 22, 2018

DeNeutoy May 23, 2018

DeNeutoy commented May 23, 2018

nelson-liu commented May 23, 2018 •

edited

Loading

schmmd commented May 24, 2018

schmmd May 25, 2018

schmmd May 25, 2018

nelson-liu May 25, 2018 •

edited

Loading

schmmd May 25, 2018

schmmd left a comment

schmmd May 25, 2018

nelson-liu May 25, 2018

schmmd May 25, 2018

nelson-liu May 25, 2018

schmmd May 25, 2018

matt-gardner May 25, 2018

nelson-liu May 25, 2018

matt-gardner May 25, 2018

schmmd May 25, 2018

nelson-liu commented May 25, 2018

schmmd commented May 25, 2018

nelson-liu commented May 25, 2018 •

edited

Loading

DeNeutoy commented May 25, 2018

DeNeutoy commented May 25, 2018

matt-gardner commented May 25, 2018

nelson-liu commented May 25, 2018

joelgrus commented May 25, 2018

nelson-liu commented May 25, 2018

		@@ -60,7 +61,7 @@ def filename_to_url(filename: str, cache_dir: str = None) -> Tuple[str, str]:

		return url, etag

		def cached_path(url_or_filename: str, cache_dir: str = None) -> str:

		from allennlp.commands.test_install import _get_module_root


		class TestTestInstall(AllenNlpTestCase):

Fix test-install by moving tests into module #1232

Fix test-install by moving tests into module #1232

Conversation

nelson-liu commented May 17, 2018 • edited Loading

nelson-liu commented May 17, 2018 • edited Loading

nelson-liu commented May 18, 2018 • edited Loading

schmmd commented May 21, 2018

nelson-liu commented May 21, 2018 • edited Loading

nelson-liu commented May 22, 2018

schmmd commented May 22, 2018

Choose a reason for hiding this comment

DeNeutoy commented May 23, 2018

nelson-liu commented May 23, 2018 • edited Loading

schmmd commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelson-liu May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelson-liu commented May 25, 2018

schmmd commented May 25, 2018

nelson-liu commented May 25, 2018 • edited Loading

DeNeutoy commented May 25, 2018

DeNeutoy commented May 25, 2018

matt-gardner commented May 25, 2018

nelson-liu commented May 25, 2018

joelgrus commented May 25, 2018

nelson-liu commented May 25, 2018

nelson-liu commented May 17, 2018 •

edited

Loading

nelson-liu commented May 17, 2018 •

edited

Loading

nelson-liu commented May 18, 2018 •

edited

Loading

nelson-liu commented May 21, 2018 •

edited

Loading

nelson-liu commented May 23, 2018 •

edited

Loading

nelson-liu May 25, 2018 •

edited

Loading

nelson-liu commented May 25, 2018 •

edited

Loading