Bug (medcat): CU-869bj3d4g Missing dependency finder doesn't work properly #271
Conversation
| req = get_required_extra_deps(package_name, extra_name) | ||
| if not req: | ||
| raise IncorrectExtraComponent(package_name, extra_name) | ||
| if len(installed) != len(req): |
There was a problem hiding this comment.
I think this will miss if you happen to have exactly the same count of deps, just not the same
Could you instead always get missing? Something like this?
| if len(installed) != len(req): | |
| missing = [requirement for requirement in req | |
| if requirement not in installed] | |
| if missing: | |
| raise MissingDependenciesError(package_name, extra_name, missing) | |
Else generally doing however set difference should work in python anyway...
There was a problem hiding this comment.
The current implementation guarantees that if the lengths are the same, then the installed dependencies are correct. That's because the get_installed_extra_dependencies method uses get_required_extra_deps insternally and only returns a sub-list that contains the deps that are actually installed.
With that said, changing these to sets and doing a set comparison may be a more robust option overall.
| self, | ||
| mock_get_required_extra_deps, | ||
| mock_get_installed_extra_dependencies): | ||
| mock_get_installed_extra_dependencies.return_value = ["A", "B", "C"] |
There was a problem hiding this comment.
Would like an extra test for ["A,B"] vs ["B", "C"] just to assert that it isnt just length of deps
alhendrickson
left a comment
alhendrickson
left a comment
There was a problem hiding this comment.
Looks good, thanks!
When a partial set of extra dependencies are installed, the current process allows the import to continue. This could happen (for instance) if the user manually installs some of the deps, or if they've installed them as part of another optional extra.
This PR will fix that by fixing that logic.
The first commit will fail due to having added the test. The subsequent commit will succeed since the logic is fixed.
EDIT:
Turned out I also had to do a remap of
scikit-learntosklearnfor the checks.