-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix test-install by moving tests into module #1232
Conversation
nevermind this still doesn't solve the fundamental issue of having no way to reliably access |
c90a4e8
to
68483cd
Compare
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 |
61dc8fd
to
4900696
Compare
@nelson-liu what's the difference between |
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... |
e4d9622
to
80219c7
Compare
(let me know if there's anything I can do to make this easier to review, as well). |
@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. |
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelgrus does this Union
, and all the str(path)
calls in this PR change how you feel about Path
?
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? |
NumPy / SciPy lets users run their tests through the CLI (https://stackoverflow.com/a/9200923/2544124):
(and accordingly for scipy) Also theano |
721626f
to
a1c903c
Compare
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 |
a1c903c
to
aa62a9e
Compare
aa62a9e
to
5cddce8
Compare
from allennlp.commands.test_install import _get_module_root | ||
|
||
|
||
class TestTestInstall(AllenNlpTestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that helps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nelson-liu is this an intentional print
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not, thanks for catching that!
[mypy] | ||
|
||
[mypy-allennlp.tests*] | ||
ignore_errors = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? Did mypy
previously not run on our tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, mypy was previously not being run on the testing code --- I thought this was intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you made the right change in this PR then. I'm not sure what we want long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah, ok, I'll open that as an issue. Thanks for doing all of this!
from allennlp.commands.test_install import _get_module_root | ||
|
||
|
||
class TestTestInstall(AllenNlpTestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
@nelson-liu sounds great. If you can move |
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. |
I was semi-serious about not liking the |
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. |
@DeNeutoy, I agree that the only benefit I see to using the |
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 |
to @DeNeutoy 's point, calling (my ideal solution would be that those places accept Path objects tho) |
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.
Goals of this PR:
python setup.py
. Currently, pip copies them tosite-packages
, which is definitely not intended behavior.scripts
ortutorials
). The issue is that files can be in different places depending on if allennlp is installed withpip
orsetup.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 usingtest-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 usingallennlp test-install
to run tests intutorials
, 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 intest-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