-
Notifications
You must be signed in to change notification settings - Fork 625
Update compatibility matrix files #2400
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
base: main
Are you sure you want to change the base?
Conversation
75ad8f3
to
ac77130
Compare
Also required updating compatgen with `manylinux_2`
Generate updated compatibility matrix files This required fixing the helper that fetched images from nvidia/cuda. Image tags no longer include the CuDNN version in the tag, which requires fetching the image config and parsing the version out of ENV.
ac77130
to
dc9197c
Compare
@@ -14,12 +14,12 @@ type Version struct { | |||
} | |||
|
|||
func NewVersion(s string) (version *Version, err error) { | |||
// TODO[md]: handle prerelease versions (0.1.2-rc1) so they aren't appended to the previous component | |||
// todo[md]: tbh just switch to hashicorp/go-version or github.com/Masterminds/semver/v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 ive used semver/v3 in the past, its pretty nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always go to that one too, but I found it the other day it doesn't support "invalid" semver input like ubuntu's "22.04" with leading zeros. I was hoping to use the same version code in cog and the new base image generator code 😒
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one question, but overall lgtm
if len(parts) != 4 { | ||
return nil, fmt.Errorf("Tag must be in the format <cudaVersion>-cudnn<cudnnVersion>-{devel,runtime}-ubuntu<ubuntuVersion>. Invalid tag: %s", tag) | ||
func parseCUDABaseImage(ctx context.Context, tag string) (*config.CUDABaseImage, error) { | ||
fmt.Println("parsing", tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug printlns ? / do we want to keep these ?
tools/compatgen/internal/cuda.go
Outdated
images := make([]config.CUDABaseImage, len(tags)) | ||
eg, egctx := errgroup.WithContext(context.TODO()) | ||
// set a concurrency limit to avoid throttling by the docker hub api (since these are authenticated requests) | ||
eg.SetLimit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use error group at all then if we are running them serially?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
natural evolution of intermittent issues and sloppy code. fixing :)
@markphelps I removed the unnecessary errgroup and excessive print statements. I left one for each image since the process takes a few minutes and it's nice to see some output to know it's not hanging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Generate updated compatibility matrix files to handle recent changes in #2376 and #2099 which default to python 3.13
This required 2 supporting fixes:
@8W9aG do we need to do anything to base images for this?