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

bump: ⬆️ CUDA Version #1769

Merged
merged 6 commits into from
Nov 18, 2024
Merged

Conversation

odulcy-mindee
Copy link
Collaborator

Fix #1743

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.55%. Comparing base (83f1bc5) to head (84325af).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1769      +/-   ##
==========================================
- Coverage   96.56%   96.55%   -0.02%     
==========================================
  Files         164      165       +1     
  Lines        7895     7901       +6     
==========================================
+ Hits         7624     7629       +5     
- Misses        271      272       +1     
Flag Coverage Δ
unittests 96.55% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Dockerfile Outdated Show resolved Hide resolved
@felixdittrich92 felixdittrich92 added this to the 0.11.0 milestone Nov 6, 2024
@felixdittrich92 felixdittrich92 added topic: ci Related to CI topic: docker Docker-related type: misc Miscellaneous labels Nov 6, 2024
@odulcy-mindee
Copy link
Collaborator Author

@felixdittrich92 I checked disk usage for both images, I don't think it's worthwhile to make a distinction for base image:

ubuntu                             22.04                                      97271d29cb79   2 months ago    77.9MB
nvidia/cuda                        12.4.0-base-ubuntu22.04                    adcf2ab568f0   7 months ago    244MB

So I only kept nvidia/cuda:12.2.0-base-ubuntu22.04 as base image.
Moreover, with the current docTR setup, torch always install nvidia packages, so I decided to do the same with TensorFlow, which leads to simplification in the CI.
I also set CUDA version to 12.2. It's an arbitrary choice. It should be compatible enough for most setup.

Tell me what you think about this setup, especially the suggested changes I made in pyproject.toml

@felixdittrich92
Copy link
Contributor

@felixdittrich92 I checked disk usage for both images, I don't think it's worthwhile to make a distinction for base image:

ubuntu                             22.04                                      97271d29cb79   2 months ago    77.9MB
nvidia/cuda                        12.4.0-base-ubuntu22.04                    adcf2ab568f0   7 months ago    244MB

So I only kept nvidia/cuda:12.2.0-base-ubuntu22.04 as base image. Moreover, with the current docTR setup, torch always install nvidia packages, so I decided to do the same with TensorFlow, which leads to simplification in the CI. I also set CUDA version to 12.2. It's an arbitrary choice. It should be compatible enough for most setup.

Tell me what you think about this setup, especially the suggested changes I made in pyproject.toml

The pyproject.toml change is fine for me it restores the old behaviour before tf 2.18 and at the end people can still install the prefered tf version before installing doctr 👍

Yeah every CUDA 12.X should be fine :)

I'm also fine with the base image - (less CI jobs ^^) because it doesn't matter much (torch and tf are already really huge packages) - people can still build there own containers - or use OnnxTR if they want something more lightweight/hardware optimized

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@odulcy-mindee odulcy-mindee marked this pull request as ready for review November 18, 2024 10:53
Copy link
Contributor

@felixdittrich92 felixdittrich92 left a comment

Choose a reason for hiding this comment

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

@odulcy-mindee

Thanks LGTM 👍

But one thing the mypy CI job fails now due to:

ERROR: Could not install packages due to an OSError: [Errno 28] No space left on device

😅

@felixdittrich92
Copy link
Contributor

So the problem now with the updated dev dependencies is that it needs to much space for the Github standard runners:

Option 1: Increase the runners to some larger ones (ubuntu-latest-large for example everywhere in the CI where we need to install .[dev] - I think this would need a configuration if you don't use enterprise on the mindee github account)
Option 2: ?? 😅 (some hacky quick fixes would be possible - but personally i don't like one of them) 😅

@felixdittrich92
Copy link
Contributor

Ok after cleaning the action caches it works again maybe we should add a CI job to auto clean the caches:

https://docs.github.com/de/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#managing-caches

Failing mypy can be ignored the issues are already solved on main

@felixdittrich92
Copy link
Contributor

felixdittrich92 commented Nov 18, 2024

For cache cleaning we could add a CI job (other PR)

For example:

name: Clear Cache

on:
  workflow_dispatch:

on:
  schedule:
    - cron: '0 0 * * *'  # Runs once a day
  
jobs:
  clear:
    name: Clear cache
    runs-on: ubuntu-latest
    steps:
    - uses: MyAlbum/purge-cache@v2
      with:
          max-age: 172800 # Caches older then 2 days are delete

@odulcy-mindee
Copy link
Collaborator Author

@felixdittrich92 Ok, seems a good idea to add a CI Job in another PR

@odulcy-mindee odulcy-mindee merged commit 99842ba into mindee:main Nov 18, 2024
67 of 69 checks passed
@odulcy-mindee odulcy-mindee deleted the docker_bump_cuda branch November 18, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: ci Related to CI topic: docker Docker-related type: misc Miscellaneous
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docker] Update Dockerfile to CUDA 12.X
3 participants