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

Add Torchvision and Torchaudio to base images #1897

Merged
merged 6 commits into from
Aug 23, 2024

Conversation

8W9aG
Copy link
Contributor

@8W9aG 8W9aG commented Aug 22, 2024

  • Exclude torchaudio from pip installs (like torchvision)
  • Install torchvision and torchaudio in the base images

8W9aG added 2 commits August 22, 2024 12:43
* Use our torch audio from the cog base images
instead
* Add the compatible versions of torchvision and
torchaudio to the base image python packages
@andreasjansson
Copy link
Member

I don't think we should excludePackages at all. When we do that, pip doesn't know that a version is already installed. Instead I think we should add torchvision and torchaudio to the requirements if they're not there already.

We probably also need some system packages for torchaudio to work. I'm guessing ffmpeg and possible some others.

@8W9aG
Copy link
Contributor Author

8W9aG commented Aug 22, 2024

I don't think we should excludePackages at all. When we do that, pip doesn't know that a version is already installed. Instead I think we should add torchvision and torchaudio to the requirements if they're not there already.

We probably also need some system packages for torchaudio to work. I'm guessing ffmpeg and possible some others.

I'll add these packages to the base image: https://pytorch.org/audio/main/installation.html

Instead of excluding packages I will add them if they don't exist and if they exist I will overwrite them to the version we support, does that sound like a plan?

@8W9aG
Copy link
Contributor Author

8W9aG commented Aug 22, 2024

I don't think we should excludePackages at all. When we do that, pip doesn't know that a version is already installed. Instead I think we should add torchvision and torchaudio to the requirements if they're not there already.
We probably also need some system packages for torchaudio to work. I'm guessing ffmpeg and possible some others.

I'll add these packages to the base image: https://pytorch.org/audio/main/installation.html

Instead of excluding packages I will add them if they don't exist and if they exist I will overwrite them to the version we support, does that sound like a plan?

Should also add that it seems these base packages are already installed: https://github.com/replicate/cog/blob/main/pkg/dockerfile/base.go#L18

@andreasjansson
Copy link
Member

andreasjansson commented Aug 22, 2024

Another thing that occurred to me: Are we including the right index url in the generated requirements.txt for the user image when we include torch?
image

Might warrant a test if no test is currently exercising that.

8W9aG added 2 commits August 22, 2024 16:37
* If we don’t find a base image explicitly set
this flag to false if it was ambiguous before
* This allows the rest of the script to continue
as if the user does not want a base image
@8W9aG
Copy link
Contributor Author

8W9aG commented Aug 23, 2024

Another thing that occurred to me: Are we including the right index url in the generated requirements.txt for the user image when we include torch? image

Might warrant a test if no test is currently exercising that.

We weren't but I've got tests now that correct for that

Copy link
Member

@andreasjansson andreasjansson left a comment

Choose a reason for hiding this comment

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

👍

@8W9aG 8W9aG merged commit b76869f into main Aug 23, 2024
15 checks passed
@8W9aG 8W9aG deleted the add-torchvision-torchaudio-base-image branch August 23, 2024 14:55
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.

2 participants