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

Remove constraints from GitHub in constraints.txt #2364

Merged
merged 2 commits into from Nov 6, 2023
Merged

Conversation

spencerkclark
Copy link
Member

@spencerkclark spencerkclark commented Nov 3, 2023

#2267 added torch_harmonics via a GitHub URL to the pip-requirements.txt file, which subsequently led to it being used in the constraints.txt file. This broke the dataflow image build as newer versions of pip no longer allow this: #2304.

This PR implements the fix suggested in a discussion between @frodre and @oliverwm1 in Slack a while back:

Could we just avoid having it in the constraints entirely? It would still get installed in the image right?

Indeed it seems like even if older versions of pip allow GitHub URLs to appear in constraints files, they do not have the desired effect. In other words if one does:

$ pip install -c constraints.txt torch_harmonics

in an environment without it already installed, pip will download the latest version off of PyPI instead of fetching it from the GitHub URL in the constraints file (perhaps this is not surprising since it is unnamed and pip is not set up to infer the name from the URL). Thus I suppose it is somewhat misleading to include GitHub URLs in the constraints to begin with, and sufficient / better that we specify torch_harmonics from GitHub in the setup.py in xtorch_harmonics:

"torch_harmonics @ git+https://github.com/NVIDIA/torch-harmonics.git@8826246cacf6c37b600cdd63fde210815ba238fd", # noqa: E501

All that being said, I have implemented this in a way such to retain git+https://github.com/NVIDIA/torch-harmonics.git@8826246cacf6c37b600cdd63fde210815ba238fd in pip-requirements.txt since it appears pip-compile can still resolve dependencies from packages on GitHub and use that to inform the versions of other packages pinned in the constraints.txt file, which is still useful.

Requirement changes:

Resolves #2304

Coverage reports (updated automatically):

@spencerkclark spencerkclark changed the title Remove constraints from GitHub in dataflow image build Remove constraints from GitHub in pip-requirements.txt Nov 3, 2023
@spencerkclark spencerkclark changed the title Remove constraints from GitHub in pip-requirements.txt Remove constraints from GitHub in constraints.txt Nov 3, 2023
In testing, it appears that unnamed constraints don't really have an effect on
what pip installs anyway, so they only seem to cause problems if included in
the constraints.txt file.

We retain dependencies from GitHub in requirements files passed to pip-compile,
as it can resolve dependencies of GitHub-based packages and use that to inform
the versions pinned.
Copy link
Contributor

@oliverwm1 oliverwm1 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. One minor suggestion.

Makefile Outdated Show resolved Hide resolved
Co-authored-by: Oliver Watt-Meyer <oliverwatt@gmail.com>
@spencerkclark spencerkclark enabled auto-merge (squash) November 6, 2023 17:45
@spencerkclark spencerkclark merged commit 0de3108 into master Nov 6, 2023
11 of 12 checks passed
@spencerkclark spencerkclark deleted the fix-dataflow branch November 6, 2023 18:14
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.

Constraints from GitHub break dataflow image build
2 participants