Import integration libraries first #7650
Conversation
There was a problem hiding this comment.
Thanks for the PR and sorry to have moved your imports. Will remember that for next time (though not sure there will be a next time since I'm done with the Trainer cleanup :-) )
While I understand the need for testing, I'm not sure about adding integrations to our "testing" dependency env (and have those tests for integration run on every commit). Maybe creating another dependency env that we would install on our slow tests would be better.
setup.py
Outdated
| @@ -90,7 +90,9 @@ | |||
| extras["all"] = extras["serving"] + ["tensorflow", "torch"] | |||
|
|
|||
| extras["retrieval"] = ["faiss-cpu", "datasets"] | |||
| extras["testing"] = ["pytest", "pytest-xdist", "timeout-decorator", "parameterized", "psutil"] + extras["retrieval"] | |||
| extras["testing"] = ["pytest", "pytest-xdist", "timeout-decorator", "parameterized", "psutil", "comet_ml"] + extras[ | |||
There was a problem hiding this comment.
I'm not fond of adding integrations to the "testing" extra dependency as they are already quite a few of them (between logging and hyperparameter search) and it would then slow down our CI quite a bit to install everything (on every commit).
We can add a "testing-integrations" flag if we want to add some tests that we run on a slower schedule (like once a day).
There was a problem hiding this comment.
Totally agree! I think we'll create a new repo for testing, and run often so we can catch errors likes this. Shall I remove the test commits, or do you want to cherry pick?
There was a problem hiding this comment.
If you can remove them it's easier for me to merge :-)
There was a problem hiding this comment.
No problem, will do.
There was a problem hiding this comment.
All done! (since I touched setup.py, black insists on reformatting it. Not sure how to get it back to original state without re-running black on it, without creating a new PR.)
There was a problem hiding this comment.
As long as the CI is happy, I'm good with the change ;-) Thanks!
* Import intergration libraries first * isort and black happiness * flake8 happiness * Add a test * Black reformat * Ignore import order in tests * A heavy-handed method of disabling comet for tests * Remove comet_ml tests * Run black on setup.py
This reverts commit 8c0e8ae.
What does this PR do?
This PR restores the order of importing 3rd-party integrations before other ML frameworks, and before any other transformer modules.
Before PR:
After PR: