-
Notifications
You must be signed in to change notification settings - Fork 244
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
@W-17427085: Set ANNOY related dependencies to be optional #3858
@W-17427085: Set ANNOY related dependencies to be optional #3858
Conversation
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.
Looks good to me
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.
Thanks for the updates, @aditya-balachander. The changes to make the optional dependencies conditional look good, but I recommend adding a new CI build that installs and tests with all optional dependencies (select
). This will:
- Make sure everything works when optional dependencies are installed together.
- Catch compatibility issues early.
You can use the uv sync --all-extras
command to install all optional dependencies. Let me know if you need help configuring the CI build to include this.
Hey @jstvz, Have added the additional workflow. Let me know if this looks good |
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.
LGTM
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.
Looks good to me
Changes:
"annoy", "numpy", "pandas", "scikit-learn"
from dependencies underpyproject.toml
and add them under optional dependenciesOPTIONAL_DEPENDENCIES_AVAILABLE
, to indicate if ANNOY related dependencies are present inselect_utils.py
. If these optional dependencies are not available, for high volume of records (i.e.complexity_constant >= 1000
), still Levenshtein Distance based selection will apply.pandas
and ANNOY related optional dependencies undertest_select_utils.py