Skip to content
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

Dev/switch org api #541

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def unique_flatten_dict(d):
}

base_extras_heavy = {
'umap-learn': ['umap-learn', 'dirty-cat==0.2.0', 'scikit-learn==1.2.2'],
'umap-learn': ['umap-learn', 'dirty-cat==0.2.0', 'scikit-learn==1.3.2'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not edit dependencies without a specific clear reason

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(pydata GPU dependencies are fragile and require substantial testing for changes )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmeyerov , checked with my colleague, he pinned this dependencies because there is an exception thrown with latest scikit-learn

TypeError: _iter() missing 3 required positional arguments: 'column_as_labels', 'skip_drop', and 'skip_empty_columns'

pinning to version 1.3.2 can prevent this error.
The error is coming from dirty_cat SuperVectorizer that was using scikit-learn. But SuperVectorizer is depreciated and replaced with TableVectorizer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already pinned, not latest

Copy link
Contributor

@lmeyerov lmeyerov Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove from this PR either way, if worthwhile, it should go in its own PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll remove it.
it will cause the test script failure, therefore, we pin it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't pin, test script will fail

Copy link
Contributor

@lmeyerov lmeyerov Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is about tests, we can patch test (non-prod) deps?

Working through a prod dep here would take more work to chase down, eg, GPU RAPIDS alignment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From discussion:

  • we can revert to original of scikit > 1.0...
  • ... and in testai cfg, do a <,> to skip the problematic recent scikits (I forgot exact syntax):

}
# https://github.com/facebookresearch/faiss/issues/1589 for faiss-cpu 1.6.1, #'setuptools==67.4.0' removed
base_extras_heavy['ai'] = base_extras_heavy['umap-learn'] + ['scipy', 'dgl', 'torch<2', 'sentence-transformers', 'faiss-cpu', 'joblib']
Expand Down
Loading