Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add optional dependency management #537
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
Add optional dependency management #537
Changes from 15 commits
d8d6c6b
2915ccc
702c11e
317806a
0b8f869
a56e6ad
fc7618b
1196c3b
642de25
a7c883d
99d697a
94aaf32
0df1ba6
3d7a122
8410242
026133f
8407df4
2ff55ae
b9385d7
5bc3119
60b97c6
ece444f
1e211c4
9c31d66
5e8ed81
67c81b4
b9458eb
3375873
6426b61
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
should this be
'pytorch': ['torch']
? check elsewhere whereBackendValidator
is invoked too and the docstring forBackendValidator
also.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.
The dependencies' presence is checked by the
HAS_BACKEND
dict
which has keypytorch
. Thepip install
message on the other hand is generated from theERROR_TYPES
which maps totorch
which is also the name of our dependency bucket. So the error message is:I kept this in line with the error messages that were being generated prior to the addition of the BackendValidator.
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.
ok makes sense, but probably would be great to add some comments in the code around this
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.
@mauicv did you add the comments @jklaise is referring to here?
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've updated the comment here to be clearer about the behaviour!
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.
Similarly
'sklearn': ['scikit-learn']
? Although I tested this and it seems they aliasedsklearn
to pull inscikit-learn
.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.
In fact,
scikit-learn
is a core dependency, not sure why this check is even here.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 copied the functionality from here and here. But yeah, I don't think it makes sense to have the full import check so I've just set
HAS_BACKEND['sklearnn] = True
. I think It is simpler just to keep the general logic in place even though it's redundant. Although happy to change?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.
Not sure about your comment about the full import check vs setting the boolean flags, probably would need to see the code.
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.
Just that here we try and import
sklearn
and then sethas_sklearn
conditional on the result which is redundant assklearn
should always be there!