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

Move ml deps into extras #121

Closed
wants to merge 1 commit into from
Closed

Conversation

kalekseev
Copy link

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] while pip 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?

@mailgun-ci
Copy link

Can one of the admins verify this patch?

@mnba
Copy link

mnba commented Dec 4, 2016

And a lot of people would like to use (and now really use) talon with ML
and it would be better someone repair talon.signature cross-circular dependency
while not removing/extracting ML part only because of corrupt part

I proposed solution to that in some comment here, while didn't provide PR

@kalekseev
Copy link
Author

kalekseev commented Dec 4, 2016

@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.
I don't expect this PR will be accepted in the current state. This is more a proof of concept that ml can be moved into the extras and my proposal to work on this if authors agree to make this change.

@obukhov-sergey
Copy link
Member

@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 --no-ml option. Here's the commit.

@kalekseev
Copy link
Author

@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
Even the author of the --no-ml patch doesn't use this option anymore zulip/zulip@624f79a#diff-dc7e3ad1b4e905601b3a4311ceb1f5ec

andersk added a commit to andersk/talon that referenced this pull request Oct 3, 2019
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>
andersk added a commit to andersk/talon that referenced this pull request Oct 3, 2019
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>
andersk added a commit to andersk/talon that referenced this pull request Jul 30, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants