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

Clean & document dependencies #69

Merged
merged 12 commits into from
May 6, 2020
Merged

Clean & document dependencies #69

merged 12 commits into from
May 6, 2020

Conversation

radujica
Copy link
Contributor

@radujica radujica commented May 6, 2020

Ideally, we would just add per-OS requirements in requirements.txt, e.g.

torch==1.6.0+cpu -f https://download.pytorch.org/whl/torch_stable.html; platform_system == "Windows"

but -f does not seem to work inside requirements.txt atm (pypa/pip#8103)

@radujica radujica marked this pull request as ready for review May 6, 2020 09:21
@radujica radujica requested review from hrzn and TheMP as code owners May 6, 2020 09:21
@@ -33,6 +33,8 @@ jobs:

- name: "5. Install main and dev dependencies"
run: |
# this avoids an unnecessary cuda install on Linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we can put it in some requirements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, see PR:

  • easy install steps for all envs
  • need fbprophet to 0.6 because they fix an easter 'egg' (facebook/prophet#1300)
  • make sure CI uses non-cuda torch

Ideally, we would just add per-OS requirements in requirements.txt, e.g.

torch==1.6.0+cpu -f https://download.pytorch.org/whl/torch_stable.html; platform_system == "Windows"

but -f does not seem to work inside requirements.txt atm (pypa/pip#8103)

and [torch](https://pytorch.org/get-started/locally/), then skip to
[Install u8timeseries](#install-u8timeseries)

To create a conda environment for Python 3.7
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put 3.6 as it is our min requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, though it's just an example for users that never used conda

conda install -c conda-forge -c pytorch pip fbprophet pytorch cpuonly

## Install u8timeseries
pip install u8timeseries
Copy link
Contributor

Choose a reason for hiding this comment

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

Running only this one out-of-the-box in a non-conda virtualenv doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, because of the non-Python platform-specific stuff, in this case fbprophet (and indirectly pystan) and possibly more.

Essentially everything in the Dockerfile is needed (plus whatever is in the FROM image) and Conda handles it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, bummer :/

@radujica radujica merged commit 9abe67f into develop May 6, 2020
@radujica radujica deleted the feature/remove_deps branch May 6, 2020 16:04
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