Optional dependencies #1798
Optional dependencies #1798
Conversation
| @@ -37,6 +37,7 @@ install_requires= | |||
| # sqlitedict==2.0.0 is slow! https://github.com/RaRe-Technologies/sqlitedict/issues/152 | |||
| # Keep sqlitedict version at 1.7.0. | |||
| sqlitedict~=1.7.0 | |||
| bottle~=0.12.23 | |||
There was a problem hiding this comment.
Why is bottle not under server?
There was a problem hiding this comment.
I renamed that section from "server" to "proxy-server". Bottle is not under there because it's also used for helm-server, not just the proxy.
| @@ -358,6 +359,12 @@ def filter_by_metadata(self, subset: ICESubset, header_dir: str, all_corpus_file | |||
| return [] | |||
|
|
|||
| for fi, columns in files: | |||
|
|
|||
| try: | |||
| import xlrd # noqa | |||
There was a problem hiding this comment.
Why not do this at the top-level like the other imports of optional dependencies?
There was a problem hiding this comment.
Moved. I wanted to keep it close to the pd.read_excel() call for readability, but I agree that it is better to have it at the top and just add a comment to explain it.
| # TODO: Ask user to install more specific optional dependencies | ||
| # e.g. crfm-helm[plots] or crfm-helm[server] | ||
| raise OptionalDependencyNotInstalled( | ||
| f"Optional dependency {e.name} is not installed. " "Please run `pip install helm-crfm[all]` to install it." |
There was a problem hiding this comment.
We could ask them to look at setup.cfg to determine which optional dependencies to install rather than always defaulting to [all].
There was a problem hiding this comment.
Yup, I have an idea of how to suggest which command to run, but I'll do it in the next PR.
yifanmai
force-pushed
the
yifanmai/fix-optional-dependencies-round-2
branch
from
727ea4f to
83f1aee
Compare
If a user attempts to use a feature that requires an optional dependency, they will be prompted to run
pip install crfm-helm[all].Addresses #1327