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

Drop support for RedisAI 1.2.5 #383

Merged
merged 10 commits into from
Oct 4, 2023
Merged

Drop support for RedisAI 1.2.5 #383

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e930cbb
Drop support for RedisAI 1.2.5
al-rigazzi Oct 3, 2023
bae8108
Address reviewers' comments
al-rigazzi Oct 4, 2023
8871ee9
Unify RAI v1.2.7 checkout
al-rigazzi Oct 4, 2023
675e53f
Modify error message
al-rigazzi Oct 4, 2023
f991339
Update changelog.
al-rigazzi Oct 4, 2023
f59b47d
Fix broken PR link in changelog
al-rigazzi Oct 4, 2023
2cd4276
Make changelog entry more precise
al-rigazzi Oct 4, 2023
8f7a56a
Fix commit link for RAI
al-rigazzi Oct 4, 2023
546abf3
Update to new SR API
al-rigazzi Oct 4, 2023
8742857
Fix ONNX issue on Mac OSX
al-rigazzi Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions .github/workflows/run_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,9 @@ jobs:
matrix:
os: [macos-12, ubuntu-20.04] # Operating systems
compiler: [8] # GNU compiler version
rai: [1.2.5, 1.2.7] # Redis AI versions
rai: [1.2.7] # Redis AI versions
py_v: [3.8, 3.9, '3.10'] # Python versions
exclude:
# Do not build with Redis AI 1.2.5 on MacOS
- os: macos-12
rai: 1.2.5
# Do not build Redis AI 1.2.5 with py3.10
# as wheels for dependecies are not availble
- py_v: '3.10'
rai: 1.2.5


env:
SMARTSIM_REDISAI: ${{ matrix.rai }}
Expand All @@ -86,7 +79,7 @@ jobs:
echo "$(brew --prefix)/opt/make/libexec/gnubin" >> $GITHUB_PATH

- name: Build Singularity from source
if: contains( matrix.os, 'ubuntu' ) && matrix.py_v == 3.9 && matrix.rai == '1.2.5'
if: contains( matrix.os, 'ubuntu' ) && matrix.py_v == 3.9
run: |
sudo apt-get install -y libseccomp-dev pkg-config squashfs-tools cryptsetup curl git # wget build-essential
echo 'export PATH=/usr/local/go/bin:$PATH' >> ~/.bashrc
Expand All @@ -100,7 +93,7 @@ jobs:
sudo make -C builddir install

- name: singularity pull test container # This lets us time how long the pull takes
if: contains( matrix.os, 'ubuntu' ) && matrix.py_v == 3.9 && matrix.rai == '1.2.5'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this docker image use an old version of pytorch we need to upgrade (1.9.1 instead of 1.11):

https://github.com/CrayLabs/SmartSim/blob/develop/docker/testing/Dockerfile

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think you are right. We need to update the container too before we release.

if: contains( matrix.os, 'ubuntu' ) && matrix.py_v == 3.9
run: singularity pull docker://alrigazzi/smartsim-testing

# Note: The develop branch of smartredis is installed first to ensure that any tests that depend
Expand All @@ -119,8 +112,6 @@ jobs:
run: smart build --device cpu -v

- name: Run mypy
# TF 2.6.2 has a dep conflict with new mypy versions
if: (matrix.rai != '1.2.5')
run: |
python -m pip install .[mypy]
make check-mypy
Expand Down
12 changes: 0 additions & 12 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,18 +649,6 @@ from C, C++, Fortran and Python with the SmartRedis Clients:
<td>ONNX</td>
<td>1.11.x</td>
</tr>
<td rowspan="3">1.2.5</td>
<td>PyTorch</td>
<td>1.9.x</td>
</tr>
<tr>
<td>TensorFlow\Keras</td>
<td>2.6.x</td>
</tr>
<tr>
<td>ONNX</td>
<td>1.9.x</td>
</tr>
</tbody>
</table>

Expand Down
8 changes: 1 addition & 7 deletions doc/installation_instructions/basic.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,13 @@ Supported Versions


Native support for various machine learning libraries and their
versions is dictated by our dependency on RedisAI_ 1.2.7. Users
can also select RedisAI 1.2.5 (though that also limits
the version of the ML libraries).
versions is dictated by our dependency on RedisAI_ 1.2.7.

+------------------+----------+-------------+---------------+
| RedisAI | PyTorch | Tensorflow | ONNX Runtime |
+==================+==========+=============+===============+
| 1.2.7 (default) | 1.11.0 | 2.8.0 | 1.11.1 |
+------------------+----------+-------------+---------------+
| 1.2.5 | 1.9.0 | 2.6.0 | 1.9.0 |
+------------------+----------+-------------+---------------+

TensorFlow_ 2.0 and Keras_ are supported through `graph freezing`_.

Expand Down Expand Up @@ -304,5 +300,3 @@ Build the SmartRedis library
============================

.. include:: ../../smartredis/doc/install/lib.rst


2 changes: 1 addition & 1 deletion doc/installation_instructions/platform/olcf-summit.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ directory from which these instructions are executed.
# install PyTorch and TensorFlow backend for the Orchestrator database.
export Torch_DIR=/ccs/home/$USER/.conda/envs/$ENV_NAME/lib/python3.9/site-packages/torch/share/cmake/Torch/
export CFLAGS="$CFLAGS -I/ccs/home/$USER/.conda/envs/$ENV_NAME/lib/python3.9/site-packages/tensorflow/include"
export SMARTSIM_REDISAI=1.2.5
export SMARTSIM_REDISAI=1.2.7
export Tensorflow_BUILD_DIR=/ccs/home/$USER/.conda/envs/$ENV_NAME/lib/python3.9/site-packages/tensorflow/
smart build --device=gpu --torch_dir $Torch_DIR --libtensorflow_dir $Tensorflow_BUILD_DIR -v

Expand Down
2 changes: 0 additions & 2 deletions smartsim/_core/_cli/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ def check_py_onnx_version(versions: Versioner) -> None:
f"Python {py_version.major}.{py_version.minor}. "
"Instead consider using Python 3.8 or 3.9 with Onnx "
)
if sys.platform == "linux":
msg += "1.2.5 or "
Comment on lines -70 to -71
Copy link
Contributor

Choose a reason for hiding this comment

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

Your removal makes sense but I'm confused by the underlying error message. Shouldn't we be saying that 1.2.7 applies to the redisAI version not the ONNX version?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that is a typo that I just noticed and asked Al to fix up while he is here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, see other comment on modified file - I am a bit confused.

msg += "1.2.7."
Copy link
Member

Choose a reason for hiding this comment

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

Since we no longer have branching logic based on OS, would you mind folding this into a single str to avoid the extra bin op and alloc?

"Instead consider using Python 3.8 or 3.9 with RedisAI 1.2.7"
#                                              ^^^^^^^^^^^^^
# Also could fix this `Onnx` vs `RAI` typo, while your here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, wait, here there is something I don't understand (and I think @mellis13 also mentioned this): if a user is here, RedisAI 1.2.7 is being used (we do not support other versions), but the problem is that ONNX is not compatible with Python. Should we put the ONNX version (1.11) instead of the RedisAI version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mellis13 @MattToast unifying warning message thread

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes -- I think we should specify the onnx version. Shouldn't the error message be:

"An onnx wheel is not available for "
f"Python {py_version.major}.{py_version.minor}. "
"Instead consider using Python 3.8 or 3.9 for ONNX 1.11 support"

Copy link
Member

Choose a reason for hiding this comment

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

^^ Agreed! lets use that

raise SetupError(msg)
_check_packages_in_python_env(
Expand Down
15 changes: 0 additions & 15 deletions smartsim/_core/_install/buildenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,17 +189,6 @@ class RedisAIVersion(Version_):
"""

defaults = {
"1.2.5": {
"tensorflow": "2.6.2",
"onnx": "1.9.0",
"skl2onnx": "1.10.3",
"onnxmltools": "1.10.0",
"scikit-learn": "1.0.2",
"torch": "1.9.1",
"torch_cpu_suffix": "+cpu",
"torch_cuda_suffix": "+cu111",
"torchvision": "0.10.1",
},
"1.2.7": {
"tensorflow": "2.8.0",
"onnx": "1.11.0",
Expand All @@ -214,14 +203,10 @@ class RedisAIVersion(Version_):
}
# Remove options with unsported wheels for python>=3.10
if sys.version_info >= (3, 10):
defaults.pop("1.2.5")
defaults["1.2.7"].pop("onnx")
defaults["1.2.7"].pop("skl2onnx")
defaults["1.2.7"].pop("onnxmltools")
defaults["1.2.7"].pop("scikit-learn")
# Remove incompatible RAI versions for OSX
if sys.platform == "darwin":
defaults.pop("1.2.5", None)

def __init__(self, vers: str) -> None: # pylint: disable=super-init-not-called
min_rai_version = min(Version_(ver) for ver in self.defaults)
Expand Down
18 changes: 0 additions & 18 deletions smartsim/_core/_install/builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,21 +351,6 @@ def build_onnx(self) -> bool:
def fetch_onnx(self) -> bool:
return self.build_onnx

def copy_tf_cmake(self) -> None:
"""Copy the FindTensorFlow.cmake file to the build directory
as the version included in RedisAI is out of date for us.
Note: opt/cmake/modules removed in RedisAI v1.2.5
"""
# remove the previous version
tf_cmake = self.rai_build_path / "opt/cmake/modules/FindTensorFlow.cmake"
tf_cmake.resolve()
if tf_cmake.is_file():
tf_cmake.unlink()
# copy ours in
self.copy_file(
self.bin_path / "modules/FindTensorFlow.cmake", tf_cmake, set_exe=False
)

def symlink_libtf(self, device: str) -> None:
"""Add symbolic link to available libtensorflow in RedisAI deps.

Expand Down Expand Up @@ -476,9 +461,6 @@ def build_from_git(self, git_url: str, branch: str, device: str = "cpu") -> None
checkout_osx_fix, out=subprocess.DEVNULL, cwd=self.rai_build_path
)

# copy FindTensorFlow.cmake to RAI cmake dir
self.copy_tf_cmake()

# get RedisAI dependencies
dep_cmd = self._rai_build_env_prefix(
with_pt=self.build_torch,
Expand Down