Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Do we still need the test-install command? #4252

Closed
epwalsh opened this issue May 18, 2020 · 4 comments · Fixed by #4264
Closed

Do we still need the test-install command? #4252

epwalsh opened this issue May 18, 2020 · 4 comments · Fixed by #4264

Comments

@epwalsh
Copy link
Member

epwalsh commented May 18, 2020

allennlp test-install just runs pytest in a roundabout way, and adds extra maintenance work since we have to make sure this function stays up-to-date with our pytest configuration in Makefile and pytest.ini.

It's also redundant (in our CI at least) to be running the unit tests again, and I don't how useful this command is to end users. In theory, if they (the users) can successfully install AllenNLP on an officially supported OS and Python version, they shouldn't need to run a bunch of unit tests to verify that their installation works. They should trust that we've already done that (and we have).

At most I would suggest they just run allennlp --help to make sure the binary has been installed and torch and what-not can be successfully loaded.

@matt-gardner
Copy link
Contributor

Removing it would also let us have a more typical python package layout, instead of including tests inside the library.

@schmmd
Copy link
Member

schmmd commented May 18, 2020

@epwalsh can you find and link to the PR where we added this? We didn't add it too long ago and it would be good to know why we're circling back on this issue.

@matt-gardner
Copy link
Contributor

Added two years ago: #1213. I agree with @epwalsh that something like allennlp --help is sufficient, and test-install is quite a bit of overkill.

@epwalsh
Copy link
Member Author

epwalsh commented May 18, 2020

Instead of completely removing test-install we could just make it print out some useful info (like version, pytorch version, GPUs available, etc) so that if there are people out there that use allennlp test-install programmatically - like in part of their own CI pipeline - it won't break anything for them.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants