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

CPU option in Docker file #98

Merged
merged 6 commits into from
Apr 2, 2024
Merged

CPU option in Docker file #98

merged 6 commits into from
Apr 2, 2024

Conversation

rahul-maurya11b
Copy link
Contributor

Pull Request

Description

added condition to check for cuda availability and accordingly creates environment

Fixes #97

How Has This Been Tested?

by building image in non cuda device

  • Yes

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

  • Yes

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.57%. Comparing base (084c5ab) to head (c8bb000).

❗ Current head c8bb000 differs from pull request most recent head 4a5ce2a. Consider uploading reports for the commit 4a5ce2a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #98   +/-   ##
=======================================
  Coverage   79.57%   79.57%           
=======================================
  Files          13       13           
  Lines         426      426           
=======================================
  Hits          339      339           
  Misses         87       87           

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

Copy link
Member

@jacobbieker jacobbieker left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks for it! Just a few small tweaks if possible, then we can merge it in

- zarr
- h3-py
- numpy
- torch==2.2.2+cpu -f https://download.pytorch.org/whl/torch_stable.html
Copy link
Member

Choose a reason for hiding this comment

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

I think you can set pytorch to be cpuonly with this instead, might be less brittle than this other way

Suggested change
- torch==2.2.2+cpu -f https://download.pytorch.org/whl/torch_stable.html
- cpuonly

Dockerfile Outdated Show resolved Hide resolved
RUN conda update -n base -c defaults conda

# Check if CUDA is available and accordingly choose env
RUN cuda=$(command -v nvcc > /dev/null && echo "true" || echo "false") \
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

environment_cpu.yml Outdated Show resolved Hide resolved
Co-authored-by: Jacob Bieker <[email protected]>
environment_cuda.yml Outdated Show resolved Hide resolved
@jacobbieker jacobbieker self-requested a review April 2, 2024 09:43
Copy link
Member

@jacobbieker jacobbieker left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for this

@jacobbieker jacobbieker merged commit c80ca5b into openclimatefix:main Apr 2, 2024
1 check failed
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.

Add a docker container that works without Cuda Or update the requirements.
2 participants