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

Fix optional deps installs during CI #817

Merged
merged 3 commits into from
Nov 8, 2022
Merged

Fix optional deps installs during CI #817

merged 3 commits into from
Nov 8, 2022

Conversation

ascillitoe
Copy link
Contributor

@ascillitoe ascillitoe commented Nov 8, 2022

This PR fixes an issue with our CI failing after the ray 2.1.0 release.

Issue

ray-project/ray#29224 removed the protobuf < 4.0 bound from ray. However, tensorflow still requires protobuf < 4.0, and this was not respected by pip during our CI install, leading to:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
tensorflow 2.10.0 requires protobuf<3.20,>=3.9.2, but you have protobuf 4.21.9 which is incompatible.

This is due to this line in our ci.yaml:

python -m pip install --upgrade --upgrade-strategy eager -e .[ray]

As stated by the pip documentation for upgrade-strategy eager:

It should be noted here that pip’s current resolution algorithm isn’t even aware of packages other than those specified on the command line, and those identified as dependencies.

In other words, pip is not aware of tensorflow's requirements when installing ray in isolation, so protobuf is eagerly upgraded to >4.0.

Solutions

Current proposed solution

Including tensorflow (and other optional deps) in the problem pip install means that pip is aware of tensorflow's deps. For example:

          python -m pip install --upgrade pip setuptools wheel
          python -m pip install --upgrade --upgrade-strategy eager -r requirements/dev.txt
          python -m pip install --upgrade --upgrade-strategy eager .[tensorflow,torch,shap]
          if [ "$RUNNER_OS" != "Windows" ]; then
          # Windows support for ray is experimental (https://docs.ray.io/en/latest/installation.html#windows-support)
            python -m pip install --upgrade --upgrade-strategy eager .[tensorflow,torch,shap,ray]  # include other deps so that they are taking into account during ray install
          fi

There is some additional duplication, however this shouldn't slow down the install too much since tensorflow etc have already been installed.

Alternative solution

We can avoid the duplication of tensorflow etc in the ray pip install if we change the upgrade-strategy to only-if-needed:

          python -m pip install --upgrade pip setuptools wheel
          python -m pip install --upgrade --upgrade-strategy eager -r requirements/dev.txt
          python -m pip install --upgrade --upgrade-strategy eager .[tensorflow,torch,shap]
          if [ "$RUNNER_OS" != "Windows" ]; then
          # Windows support for ray is experimental (https://docs.ray.io/en/latest/installation.html#windows-support)
            python -m pip install --upgrade --upgrade-strategy only-if-needed .[ray]  
          fi

The downside of this is that we might not install the latest ray version (and its deps) if there were older cached dependencies...

@ascillitoe ascillitoe added the WIP This PR is a Work in Progress label Nov 8, 2022
@ascillitoe ascillitoe changed the title Add tensorflow and torch to CI install Fix optional deps installs during CI Nov 8, 2022
@ascillitoe ascillitoe removed the WIP This PR is a Work in Progress label Nov 8, 2022
@ascillitoe ascillitoe requested review from jklaise and mauicv November 8, 2022 11:48
@ascillitoe
Copy link
Contributor Author

TODO - update alibi-detect after approval.

Copy link
Contributor

@jklaise jklaise left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #817 (dc8f00d) into master (2c22f59) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #817   +/-   ##
=======================================
  Coverage   75.48%   75.48%           
=======================================
  Files          73       73           
  Lines        8477     8477           
=======================================
  Hits         6399     6399           
  Misses       2078     2078           
Flag Coverage Δ
macos-latest-3.10 75.35% <ø> (ø)
ubuntu-latest-3.10 75.35% <ø> (-0.10%) ⬇️
ubuntu-latest-3.7 75.30% <ø> (ø)
ubuntu-latest-3.8 75.34% <ø> (ø)
ubuntu-latest-3.9 75.34% <ø> (ø)
windows-latest-3.9 73.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@ascillitoe ascillitoe merged commit ad2df5f into SeldonIO:master Nov 8, 2022
ascillitoe added a commit that referenced this pull request Nov 8, 2022
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.

2 participants