-
Notifications
You must be signed in to change notification settings - Fork 285
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
Move ml deps into extras #121
Conversation
Can one of the admins verify this patch? |
And a lot of people would like to use (and now really use) talon with ML I proposed solution to that in some comment here, while didn't provide PR |
@MinasAbrahamyan The cross-circular dependency is not the point of this pr. In fact I didn't know that the problem exists. I use talon in some my projects and I don't want to donwload and compile numpy, scipy, scikit and os packages for the small amount of cases when the ml is helpful. And you can see several issues in the project there people don't want to install ml part. There is even the --no-ml flag in the setup.py which do this but it's not very convenient to use it with pip. |
@kalekseev thank you for the PR and my apologies for delay with response. It should already be possible to install / use the lib without ML support if you install with |
@obukhov-sergey I know about this option but because this is not a standard way to install optional deps in pip/python it leads to problem like this pypa/pip#3845 |
The nonstandard `setup.py install --no-ml` option didn’t work correctly with pip. We could move the ML dependencies into an extra, but that approach was previously rejected (mailgun#121): extras can’t be enabled by default, so it could have been disruptive to existing users. Instead, we split off a new talon-core package with no ML dependencies, and have talon re-export everything from it. Fixes mailgun#130; fixes mailgun#131. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
The nonstandard `setup.py install --no-ml` option didn’t work correctly with pip. We could move the ML dependencies into an extra, but that approach was previously rejected (mailgun#121): extras can’t be enabled by default, so it could have been disruptive to existing users. Instead, we split off a new talon-core package with no ML dependencies, and have talon re-export everything from it. Fixes mailgun#130; fixes mailgun#131. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
The nonstandard `setup.py install --no-ml` option didn’t work correctly with pip. We could move the ML dependencies into an extra, but that approach was previously rejected (mailgun#121): extras can’t be enabled by default, so it could have been disruptive to existing users. Instead, we split off a new talon-core package with no ML dependencies, and have talon re-export everything from it. Fixes mailgun#130; fixes mailgun#131. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
A lot of people would like to use talon without ml and it's possible to move the ml deps into extras and install it with
pip install talon[ml]
whilepip install talon
will install light version without ml.I've implemented such setup and added tox which tests both versions.
This is a breaking change so I'm interested in would you accept such change if I finish all required things for the pr?