Skip to content

Conversation

@tesfaldet
Copy link
Contributor

What does this PR do?

Fixes gpu+cpu-based installation requirements in environment.yaml (conda-based install) and requirements.txt (pip-based install). Specifically, a gpu+cpu-based installation is assumed as the default method, and so for environment.yaml the necessary channels are added as well as the pytorch-cuda package. There's also an option for a cpu-only install. Similarly for a pip-based install, requirements.txt has the --extra-index-url specifier for specifying a gpu+cpu-based install or a cpu-only install, as taken from https://pytorch.org/get-started/locally/.

Before submitting

  • Did you make sure title is self-explanatory and the description concisely explains the PR?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you test your PR locally with pytest command?
  • Did you run pre-commit hooks with pre-commit run -a command?

Did you have fun?

yus

@tesfaldet
Copy link
Contributor Author

Tests can be fixed from this PR #585

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.66%. Comparing base (9355a5d) to head (9c4f7d4).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #586   +/-   ##
=======================================
  Coverage   83.66%   83.66%           
=======================================
  Files          11       11           
  Lines         355      355           
=======================================
  Hits          297      297           
  Misses         58       58           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +4 to +5
--extra-index-url https://download.pytorch.org/whl/cu118 # gpu+cpu installation (comment this line if doing cpu-only)
# --extra-index-url https://download.pytorch.org/whl/cpu # cpu-only installation (uncomment line if doing cpu-only)
Copy link
Owner

Choose a reason for hiding this comment

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

Looking at the https://pytorch.org/get-started/locally/, only cuda 11.8 requires those lines, while 11.7 doesn't, and 11.7 is set as default when installing through normal pip install torch.

I think there's not much point in trying to maintain those instructions for requirements.txt as they seem to regulary change every couple of weeks/months. I would rather just leave this file simple and let people figure out what they need by themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right. Perhaps I can add a comment that at least notifies the user of this, so it's not too painful for them to modify requirements.txt or environment.yaml

@ashleve ashleve added the dependencies Pull requests that update a dependency file label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants