Remove `--upgrade` from `pip install` #1136

carlcsaposs-canonical · 2023-07-03T16:25:36Z

Fixes #1135

Fixes canonical#1135

sed-i · 2023-07-04T16:30:44Z

Not sure we should change priority between PYDEPS and reqs.txt.
See comment here: #1135

carlcsaposs-canonical · 2023-07-05T14:11:04Z

Not sure we should change priority between PYDEPS and reqs.txt. See comment here: #1135

This doesn't change priority, it just fixes that unpinned PYDEPS will not override pinned requirements.txt deps

lengau · 2023-07-05T17:43:07Z

I think this is reasonable, but I'll have to check for edge cases this might break. (@cmatsuoka can you think of any?)

If we make this change though, I'd like a behaviour test for this with a link to #1135 so we don't break this later.

carlcsaposs-canonical · 2023-07-05T17:59:55Z

I'd like a behaviour test for this with a link to #1135 so we don't break this later.

I'm not sure how to implement this—let me know if you'd like me to try to add a behavior test

Here's a simple example:
requirements.txt ops==2.0.0
PYDEPS for charm lib ["ops"]
ensure that ops==2.0.0 is installed

sed-i · 2023-07-05T18:52:14Z

Any idea how this should interact with version pinning specified in charm-binary-python-packages?

lengau · 2023-07-05T18:56:55Z

@carlcsaposs-canonical A spread test something like this should do it: (Caveat: I haven't run this test)

summary: pack a charm with a specific (old) dependency version

include:
  - tests/

prepare: |
  tests.pkgs install unzip
  charmcraft init --project-dir=charm
  cd charm
  echo "ops==2.0.0" > requirements.txt

  cat <<- EOF >> charmcraft.yaml
  parts:
    charm:
      charm-requirements: [requirements.txt]
  EOF

  cat <<- EOF > lib/charms/charm/v0/my_lib.py
  PYDEPS = ["ops"]
  LIBID = "my_lib"
  LIBAPI = 0
  LIBPATCH = 1
  EOF


restore: |
  pushd charm
  charmcraft clean
  popd

  rm -rf charm

execute: |
  cd charm
  charmcraft pack --verbose
  test -f charm*.charm
  unzip -p charm_*.charm venv/ops/version.py | MATCH "version = '2.0.0'"

lengau · 2023-07-05T19:27:33Z

@sed-i (I could be wrong but) that it'll install necessary updates, but not update anything not necessary for the PYDEPS.

One potential edge case here is if the first thing that requires a particular dependency is a charmlib. Take for example a requirements.txt file that says:

websocket-client==0.9.0

and a lib that requires ops. This would mean what ends up being run is:

pip install --no-binary :all: --requirement=requirements.txt
pip install --no-binary :all: ops

This would install the latest ops, bringing an updated websocket-client with it, which is probably not what's intended. If we had charmcraft run pip install --no-binary :all: --requirement=requirements.txt ops instead, that would get us an older ops that doesn't require the update to websocket-client. (Specifically, it would get us 2.0.0, which is the latest version that doesn't require that package.)

If the lib requires ops>=2.1.0 for example, the charm would then fail to build due to a dependency resolution error. This is possibly a better outcome than either the current state (versions from requirements.txt are not respected if charmlibs have dependencies) or the proposed state (versions from requirements.txt are not respected if charmlibs require updates).

Feel free to weigh in about preferred behaviours and backwards compatibility — I'll reach out to a few others to consider options too.

carlcsaposs-canonical · 2023-07-05T19:37:12Z

This would install the latest ops, bringing an updated websocket-client with it, which is probably not what's intended.

Good point! It looks like installing all the dependencies in one pip install would be best, so that any dependency conflicts fail the build

Except it seems like binary dependencies cannot be installed in the same command

carlcsaposs-canonical · 2023-07-05T19:45:51Z

Except it seems like binary dependencies cannot be installed in the same command

Charmcraft could run pip install --dry-run with all the dependencies (binary or not) to check for conflicts, then install binary dependencies separately

However, even if there aren't conflicts, later commands could still override binary dependencies

Instead of using --no-binary :all: it might be possible to use --dry-run to get the dependencies and generate lists for --no-binary and --binary-only, but that might get tricky with subdependencies

carlcsaposs-canonical · 2023-07-05T19:50:55Z

Also just want to mention that because of #1077, pip doesn't have access to the full dependency resolution

i.e. during charmcraft's build, pip install runs in a virtual environment but, during runtime, system Python packages are also available that could have dependency conflicts

carlcsaposs-canonical · 2023-07-05T19:52:23Z

Nice to have: it'd be great if whatever solution is implemented is compatible with #1115 (feature request)

lengau · 2023-07-06T14:05:24Z

charmcraft/charm_builder.py

@@ -239,26 +239,26 @@ def _install_dependencies(self, staging_venv_dir):
        with instrum.Timer("Installing all dependencies"):
            if self.binary_python_packages:
                # install python packages, allowing binary packages
-                cmd = [pip_cmd, "install", "--upgrade"]  # base command
+                cmd = [pip_cmd, "install"]  # base command


This change is breaking centos 7. I'm not sure what the best way to handle it is, so I'm going to tag in @syu-w for thoughts. My best proposal right now is to add a pip install --upgrade pip setuptools run before starting any of the conditional installs.

lengau · 2023-07-06T14:15:38Z

charmcraft/charm_builder.py

                cmd.extend(self.binary_python_packages)  # the python packages to install
                _process_run(cmd)

            if self.python_packages:
                # install python packages from source
-                cmd = [pip_cmd, "install", "--upgrade", "--no-binary", ":all:"]  # base command
+                cmd = [pip_cmd, "install", "--no-binary", ":all:"]  # base command


It looks like the Jupyter notebooks charm is failing because of this (see spread test.

Between this and the above, I'm wondering whether the right change for now might be to keep the --upgrade here and above and merge the two commands below (given the edge case we discussed in the main conversation).

I believe that'll resolve #1135 with a fairly minimal change that we can put into a 2.4 release fairly quickly, with a bigger rethink of package handling for a future release. How's that sound?

Or we can change the order of install requirement.txt and PYDEPS.

So if a pinned version is installed, pip install without --upgrade will not try to get a new version. And if the package is not in requirement.txt, it will install latest stable version.

That was my original thought, but it could leave us with invalid PYDEPS that we won't discover until runtime.

Example:

PYDEPS includes pkg_a>=2.0, and requirement.txt includes pkg_a==1.0

Ideal behaviour would probably be to error out during the charm build due to a dependency conflict. However:

Current behaviour: we get pkg_a>=2.0 and a maybe-working charm.

Alternative behaviour: we get pkg_a==1.0 and an almost-guaranteed-broken charm.

Then we need to check all deps in charmcraft without rely on pip

Or maybe we can add PYDEPS to a new requirement.txt

carlcsaposs-canonical · 2023-07-14T15:09:36Z

Superseded by #1157

carlcsaposs-canonical added 2 commits July 3, 2023 16:25

Remove --upgrade from pip install

97d25d3

Fixes canonical#1135

format test

6373de6

dragomirp mentioned this pull request Jul 4, 2023

Pin pydantic's major version canonical/grafana-agent-k8s-operator#215

Merged

Add spread test

a4812c6

Update task.yaml

de9060b

lengau reviewed Jul 6, 2023

View reviewed changes

carlcsaposs-canonical closed this Jul 14, 2023

carlcsaposs-canonical deleted the pydeps-fix branch July 14, 2023 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `--upgrade` from `pip install` #1136

Remove `--upgrade` from `pip install` #1136

carlcsaposs-canonical commented Jul 3, 2023

sed-i commented Jul 4, 2023

carlcsaposs-canonical commented Jul 5, 2023

lengau commented Jul 5, 2023

carlcsaposs-canonical commented Jul 5, 2023

sed-i commented Jul 5, 2023

lengau commented Jul 5, 2023 •

edited

Loading

lengau commented Jul 5, 2023

carlcsaposs-canonical commented Jul 5, 2023 •

edited

Loading

carlcsaposs-canonical commented Jul 5, 2023

carlcsaposs-canonical commented Jul 5, 2023 •

edited

Loading

carlcsaposs-canonical commented Jul 5, 2023

lengau Jul 6, 2023

lengau Jul 6, 2023

syu-w Jul 6, 2023

syu-w Jul 6, 2023

lengau Jul 6, 2023

syu-w Jul 6, 2023

syu-w Jul 6, 2023

carlcsaposs-canonical commented Jul 14, 2023

Remove --upgrade from pip install #1136

Remove --upgrade from pip install #1136

Conversation

carlcsaposs-canonical commented Jul 3, 2023

sed-i commented Jul 4, 2023

carlcsaposs-canonical commented Jul 5, 2023

lengau commented Jul 5, 2023

carlcsaposs-canonical commented Jul 5, 2023

sed-i commented Jul 5, 2023

lengau commented Jul 5, 2023 • edited Loading

lengau commented Jul 5, 2023

carlcsaposs-canonical commented Jul 5, 2023 • edited Loading

carlcsaposs-canonical commented Jul 5, 2023

carlcsaposs-canonical commented Jul 5, 2023 • edited Loading

carlcsaposs-canonical commented Jul 5, 2023

lengau Jul 6, 2023

Choose a reason for hiding this comment

lengau Jul 6, 2023

Choose a reason for hiding this comment

syu-w Jul 6, 2023

Choose a reason for hiding this comment

syu-w Jul 6, 2023

Choose a reason for hiding this comment

lengau Jul 6, 2023

Choose a reason for hiding this comment

syu-w Jul 6, 2023

Choose a reason for hiding this comment

syu-w Jul 6, 2023

Choose a reason for hiding this comment

carlcsaposs-canonical commented Jul 14, 2023

Remove `--upgrade` from `pip install` #1136

Remove `--upgrade` from `pip install` #1136

lengau commented Jul 5, 2023 •

edited

Loading

carlcsaposs-canonical commented Jul 5, 2023 •

edited

Loading

carlcsaposs-canonical commented Jul 5, 2023 •

edited

Loading