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

Make torch and some other dependencies optional #101

Closed
meakbiyik opened this issue Aug 1, 2021 · 5 comments
Closed

Make torch and some other dependencies optional #101

meakbiyik opened this issue Aug 1, 2021 · 5 comments
Assignees

Comments

@meakbiyik
Copy link

Hi, kudos to your work as this package will be quite useful. However, it was quite a bad surprise for me to add this package to my project dependencies and see huge and non-environment-friendly packages such as pytorch, gpytorch, llvmlite and even plotly being installed into my environment. I only plan to use some changepoint detection methods and maybe visualizations, and I highly doubt that (though, of course correct me if I am wrong) the whole torch stack is necessary for such a task. Similarly, I imagine plotly can easily be made an optional dependency as well, since matplotlib is still the de facto plotting library of python and it would not be in the interest of majority of the users to install a data analysis package and get a big python-js bundle with it.

@michaelbrundage
Copy link
Contributor

michaelbrundage commented Aug 17, 2021

Thanks for reporting this. I've fixed this in f6d9d94. pip doesn't have a way for us to keep pip install kats the default and allow users to optionally opt-out of some functionality; only the opposite. So, to get a reduced version of Kats without Pytorch, plotly, and many other packages (everything listed in test_requirements.txt), you can now use

MINIMAL=1 pip install .

from the root level of the Kats source, and in the future (after we release the next version of Kats),

MINIMAL=1 pip install kats

If you later decide you want some of those additional functionalities, you can manually install the ones you want, or install all of them with

pip install -r test_requirements.txt

@meakbiyik
Copy link
Author

@michaelbrundage thanks for the quick resolve. One question though: why not poetry? I thought it was becoming kind of an industry standard by now, and you can add both the optional tag to packages, and also bundle them with extras flexibly. Also since it is PEP 517 compliant, the whole previous pipeline with pip will continue to work.

An example from a package I have written: the CLI component is optional, so those packages are marked as optional and can be installed by specifying the cli extra during install (or after install): pip install [blah] -E cli .

@michaelbrundage
Copy link
Contributor

@meakbiyik The goal was to remove dependencies, not add a new one. However, PRs always welcome. Kats would need a pyproject.toml first, and need to install smoothly across all platforms we support.

@adamantike
Copy link
Contributor

@michaelbrundage, for people using this project and installing it within their own requirements.txt file, would you consider having a more specific environment variable, as that would be the only alternative if they want a minimal installation of kats but not of the rest of their dependencies (in case any other one also considers the MINIMAL variable)?

A small change like this one would be everything we need to avoid any undesired side effects on our installation process:

if not os.environ.get("MINIMAL", False) and not os.environ.get("MINIMAL_KATS", False):
    install_requires += extra_requires

adamantike added a commit to adamantike/Kats that referenced this issue Oct 19, 2021
As commented on
facebookresearch#101 (comment),
having an environment variable that's exclusive for `kats` avoids
installation issues on projects that have multiple other dependencies,
and install them using commands like `pip install -r requirements.txt`.

In that scenario, setting `MINIMAL_KATS` avoids unintended side effects
on any other dependency that understands the `MINIMAL` environment
variable.
@michaelbrundage
Copy link
Contributor

Thanks for the PR. Is there any reason to add a second variable, or should we just rename the one variable from MINIMAL to MINIMAL_KATS to avoid potential conflicts with any other projects?

Can you point to any examples of other projects that use the MINIMAL variable?

adamantike added a commit to adamantike/Kats that referenced this issue Oct 20, 2021
As commented on
facebookresearch#101 (comment),
having an environment variable that's exclusive for `kats` avoids
installation issues on projects that have multiple other dependencies,
and install them using commands like `pip install -r requirements.txt`.

In that scenario, setting `MINIMAL_KATS` avoids unintended side effects
on any other dependency that understands the `MINIMAL` environment
variable.
facebook-github-bot pushed a commit that referenced this issue Oct 21, 2021
Summary:
As commented on #101 (comment), having an environment variable that's exclusive for `kats` avoids installation issues on projects that have multiple other dependencies, and install them using commands like `pip install -r requirements.txt`.

In that scenario, setting `MINIMAL_KATS` avoids unintended side effects on any other dependency that understands the `MINIMAL` environment variable.

Pull Request resolved: #158

Reviewed By: iamxiaodong

Differential Revision: D31793955

Pulled By: michaelbrundage

fbshipit-source-id: 6311993fc1784bf4bcc8255c4e47ee9c6a747b52
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

No branches or pull requests

3 participants