-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revise and split requirements files #340
Conversation
99ea7b9
to
6d3d0a3
Compare
Moves dependencies defined in the root `requirements.txt` to `sharktank/` and splits out test only deps to `requirements-tests.txt` file. Dependencies only used for development / in the CI are moved to `requirements-dev.txt` and the root requirements file is now used to pull in all the deps. Furthermore, some no longer used dependencies are removed.
This will need some further work but hopefully is the right step to untangle all the deps. |
f"pytest{get_version_spec('pytest')}", | ||
f"pytest-xdist{get_version_spec('pytest-xdist')}", |
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 drop pytest-xdist
from here? It's useful for running tests. I'm not sure what the best practices are for testing
deps in extras_require
, but I could see an argument for this being minimal (just pytest
).
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.
pytest-xdist
isn't used, as pytest -n
is only called in the CI. Thus I would rather go with the minimal requirements instead of pulling in non-strictly required packages.
-r sharktank/requirements.txt | ||
-r sharktank/requirements-tests.txt | ||
-r shortfin/requirements-tests.txt | ||
-r requirements-dev.txt |
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.
Interesting... top level requirements that pulls in all the others. That seems like a good idea. I hope it works out in practice :P
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.
Our CI says yes ;) Well I hope to get rid off it at some point to be honest but this would mean to refactor the CI at the same time whereas this relaxes the required changes a little and let us iterate more easily.
Moves dependencies defined in the root
requirements.txt
tosharktank/
and splits out test only deps torequirements-tests.txt
file. Dependencies only used for development / in the CI are moved torequirements-dev.txt
and the root requirements file is now used to pull in all the deps. Furthermore, some no longer used dependencies are removed.