Drop support for RedisAI 1.2.5 #383
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #383 +/- ##
========================================
Coverage 88.34% 88.34%
========================================
Files 58 58
Lines 3587 3587
========================================
Hits 3169 3169
Misses 418 418 |
There was a problem hiding this comment.
Looks great! I have some small questions and couple of things I'm gonna ask to tack onto this PR:
- Can you update the change log for this PR now? I'd like have this documented in case anyone is currently working off of develop and I ESPECIALLY want to make sure that we do not forget to call this out when release time rolls around.
- There are a 2 pylint suppressions that needed to be made on account of the pylint version that came from TF 2.6.x. Can you remove them while you are here? They are located at
smartsim/entity/entityList.py:36andsmartsim/_core/_cli/validate.py:55.
Other than that and the comments below, this looks ready to go on my end!
smartsim/_core/_cli/build.py
Outdated
| "Instead consider using Python 3.8 or 3.9 with Onnx " | ||
| ) | ||
| if sys.platform == "linux": | ||
| msg += "1.2.5 or " | ||
| msg += "1.2.7." |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@mellis13 @MattToast unifying warning message thread
There was a problem hiding this comment.
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"
| @@ -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' | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes, I think you are right. We need to update the container too before we release.
| if sys.platform == "linux": | ||
| msg += "1.2.5 or " |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yeah, that is a typo that I just noticed and asked Al to fix up while he is here
There was a problem hiding this comment.
Yep, see other comment on modified file - I am a bit confused.
|
One more comment -- don't forget to add tags to the PR for release documentation |
|
@MattToast @al-rigazzi Should we remove the branching logic in SmartSim/smartsim/_core/_install/builder.py Line 441 in e930cbb |
al-rigazzi
added
area: build
This PR removes support for RedisAI 1.2.5. This also implies that the entries in the GitHub actions matrix is reduced.