Python Requirements Ignoring Extra Packages

[yckao]

Hello there! I’m working with Cog and specifying a requirement like transformers[torch]==0.49.0. However, I’ve noticed that the extra package (i.e., torch) is currently being ignored.

Expected Behavior

Transformers should be installed with the specified extra packages.

Description

After some research, I found this pull request:
#2160

In versions prior to this PR, using transformers[torch]==0.49.0 would raise an error in SplitPinnedPythonRequirement:

cog/pkg/config/requirements.go

Lines 85 to 91 in 60017a9

	func SplitPinnedPythonRequirement(requirement string) (name string, version string, findLinks []string, extraIndexURLs []string, err error) {
	pinnedPackageRe := regexp.MustCompile(`(?:([a-zA-Z0-9\-_]+)==([^ ]+)|--find-links=([^\s]+)|-f\s+([^\s]+)|--extra-index-url=([^\s]+))`)
	matches := pinnedPackageRe.FindAllStringSubmatch(requirement, -1)
	if matches == nil {
	return "", "", nil, nil, fmt.Errorf("Package %s is not in the expected format", requirement)
	}

Meanwhile, pythonPackageForArch treated it as an unpinned dependency, which unintentionally but worked:

cog/pkg/config/config.go

Lines 397 to 401 in 2a0763a

	name, version, findLinksList, extraIndexURLs, err := SplitPinnedPythonRequirement(pkg)
	if err != nil {
	// It's not pinned, so just return the line verbatim
	return pkg, []string{}, []string{}, nil
	}

After version 0.14.0, SplitPinnedPythonRequirement no longer raises an error, but the extra package is completely ignored:

cog/pkg/config/requirements.go

Lines 8 to 50 in 2a0763a

	// SplitPinnedPythonRequirement returns the name, version, findLinks, and extraIndexURLs from a requirements.txt line
	// in the form name[extras]==version [--find-links=<findLink>] [-f <findLink>] [--extra-index-url=<extraIndexURL>]
	func SplitPinnedPythonRequirement(requirement string) (name string, version string, findLinks []string, extraIndexURLs []string, err error) {
	pinnedPackageRe := regexp.MustCompile(`(?:([a-zA-Z0-9\-_]+)(\[[a-zA-Z0-9\-_,]+])?==([^ ]+)|--find-links=([^\s]+)|-f\s+([^\s]+)|--extra-index-url=([^\s]+))`)
	matches := pinnedPackageRe.FindAllStringSubmatch(requirement, -1)
	if matches == nil {
	return "", "", nil, nil, fmt.Errorf("Package %s is not in the expected format", requirement)
	}
	nameFound := false
	versionFound := false
	for _, match := range matches {
	if match[1] != "" {
	name = match[1]
	nameFound = true
	}
	if match[3] != "" {
	version = match[3]
	versionFound = true
	}
	if match[4] != "" {
	findLinks = append(findLinks, match[4])
	}
	if match[5] != "" {
	findLinks = append(findLinks, match[5])
	}
	if match[6] != "" {
	extraIndexURLs = append(extraIndexURLs, match[6])
	}
	}
	if !nameFound || !versionFound {
	return "", "", nil, nil, fmt.Errorf("Package name or version is missing in %s", requirement)
	}
	return name, version, findLinks, extraIndexURLs, nil
	}

[nevillelyh]

Thanks, good find! You meant transformers[torch]==4.49.0 right?

I'm reverting this in #2211, as it's no longer needed for the fix I was attempting.
IIUC transformers[torch] just adds torch as an extra dependencies right? A few things to consider down the road:

• python_packages is deprecated in favor of python_requirements, and in the future we might pass requirements as is to pip install to avoid such issues
• It's recommended to set torch==x.y.z explicitly, and this might be required in the future

Meanwhile can you try replacing [torch] with a separate entry of torch==x.y.z as a workaround?

[yckao]

Thank you for the quick response.

transformers[torch] was just an example. I understand that explicitly specifying torch==x.y.z is generally better, especially since Cog performs compatibility checks.

I also noticed that python_requirements is intended to replace python_packages, but the current implementation still seems to parse requirements.txt the same way. That’s why I opened this issue.

Moreover, I think passing requirements.txt directly to pip install is a great approach. I appreciate your work on this and look forward to seeing it implemented.

[nevillelyh]

Yeah you're correct that they still go through the same path, so this is not really solved. But we'll pass requirements.txt as is in the future (soon) to avoid having to deal with such bugs. For now we just prefer to not attempt fixing a problematic code path and cause more regression. That's why I'm proposing the workaround for now.

Somewhat related accidental discovery: we prefer (and will require) a specific torch==x.y.z anyway to prevent accidental breakage from torch upgrades, instead of [torch] pulling the latest. So we'll address that separately.