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.

Sign up for GitHub

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

Jump to bottom

Constraints from GitHub break dataflow image build #2304

Closed
spencerkclark opened this issue Jul 28, 2023 · 1 comment · Fixed by #2364
Closed

Constraints from GitHub break dataflow image build #2304

spencerkclark opened this issue Jul 28, 2023 · 1 comment · Fixed by #2364

Comments

@spencerkclark
Copy link
Member

Since #2267 was merged, which added a package installed from GitHub to our constraints.txt file, the dataflow image build (e.g. here) has been broken, producing an error when pip installing packages:

#9 0.743 DEPRECATION: Constraints are only allowed to take the form of a package name and a version specifier. Other forms were originally permitted as an accident of the implementation, but were undocumented. The new implementation of the resolver no longer supports these forms. A possible replacement is replacing the constraint with a requirement. You can find discussion regarding this at https://github.com/pypa/pip/issues/8210.
#9 0.743 ERROR: Unnamed requirements are not allowed as constraints
#9 0.878 WARNING: You are using pip version 21.2.4; however, version 23.2.1 is available.

The environment in the build has a different version of pip (21.2.4) than what we pin in our environment.yml file (20.2.4). I encountered a similar issue in the lint build, which runs on every commit in each PR, and fixed it by pinning the version pip installed in that environment to 20.2.4.

In general though, it might be nice to have a solution that worked for newer versions of pip. I think it is orthogonal to this issue, but the initial issue that prompted pinning pip to this version may have been addressed in pip-tools (jazzband/pip-tools#1300).
@brianhenn
Copy link
Contributor

Agree it would be great to get away from pinning pip; it might resolve other issues, e.g., #2266.

@spencerkclark spencerkclark mentioned this issue Nov 3, 2023
Merged
1 task
spencerkclark added a commit that referenced this issue Nov 6, 2023
@spencerkclark @oliverwm1
Remove constraints from GitHub in constraints.txt (#2364)
0de3108 
#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`:


https://github.com/ai2cm/fv3net/blob/314edd50a1d9f1fe31d14a418dbe981d16c63f1d/external/xtorch_harmonics/setup.py#L16


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:
- [x] Ran `make lock_deps/lock_pip` following these
[instructions](https://vulcanclimatemodeling.com/docs/fv3net/dependency_management.html#dependency-management)

Resolves #2304

Coverage reports (updated automatically):

---------

Co-authored-by: Oliver Watt-Meyer <oliverwatt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove constraints from GitHub in constraints.txt ai2cm/fv3net
2 participants
@spencerkclark @brianhenn