Dependency Updates #256
Dependency Updates #256
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #256 +/- ##
========================================
Coverage 85.21% 85.21%
========================================
Files 60 60
Lines 3422 3423 +1
========================================
+ Hits 2916 2917 +1
Misses 506 506
|
| logger.info("Version Information:") | ||
| vers = self.versions.as_dict() | ||
| print(tabulate(vers, headers=vers.keys(), tablefmt="github"), "\n") | ||
|
|
There was a problem hiding this comment.
This seems like useful debug info -- why did we take it out?
There was a problem hiding this comment.
Good question!! There was another block roughly 10 lines earlier (~line 146) which was making roughly same print out. I figured two copies was redundant and removed the extra.
I also meant to call that out in the PR description because I figured that would be non-obvious from the diff, but didn't because I forgot and am bad at my job, lol.
There was a problem hiding this comment.
So there was a before/after display around selecting files for the database Redis/KeyDB. Maybe that was still useful, but we're probably beyond that point now
| logger.error("RedisAI support for MacOS is broken in 1.2.4 and 1.2.5") | ||
| sys.exit(1) | ||
| if self.build_env.PLATFORM == "darwin" and device == "gpu": | ||
| raise BuildError("SmartSim does not support GPU on MacOS") |
There was a problem hiding this comment.
We still support RAI 1.2.5, so some of these checks are still needed
There was a problem hiding this comment.
Also something I should have called out in the PR description!!
We were making two very similar sets of checks in two different location: both here and in in Versioner/RedisAIVersion. I decided to try and move as many of the RAI compatibility checks into RedisAIVersioner to try and eliminate the duplication.
Now when RedisAIVersion is instantiated, it will use its knowledge of the platform it is on and the version of python it is running with to try and determine if the RedisAI/ML backends can be installed. If for whatever reason a compatibility inconstancy is found it will raise a SetupError immediately. This means that downstream we should be able to use the instanced RedisAIVersion without the need to make additional compatibility checks.
For instance this will now error out on OSX or with Python==3.10:
$ export SMARTSIM_REDISAI=1.2.5
$ smart build # same erorr raised at ``pip install`` time as well
# Shortened for brevity
smartsim._core._install.buildenv.SetupError: RedisAI version must be greater than or equal to 1.2.7But the same scenario will work on linux with Python<=3.9
$ export SMARTSIM_REDISAI=1.2.5
$ smart build
# Shortened for brevity
$ echo $?
0Do let me know if you think this is acceptable/sufficient change! I could definitely be talked into potentially rolling this back if the changes are deemed "too severe"
There was a problem hiding this comment.
This sounds reasonable to me, but let's bring it up in standup tomorrow and make sure the whole team's onboard
|
I'm good with the changes as proposed, but I'm going to hold off on formally approving until another reviewer has a look and agrees or disagrees with the solutions presented in my two unresolved comments |
|
I try to build SmartSim with Python 3.10.4 Problem No. 1: I fixed the problem by installing torch manually: Second Problem |
smartsim/_core/_cli/build.py
Outdated
| @@ -367,6 +355,11 @@ def install_torch(self, device="cpu"): | |||
|
|
|||
| def check_onnx_install(self): | |||
| """Check Python environment for ONNX installation""" | |||
| if not self.versions.ONNX: | |||
| raise SetupError( | |||
| "An onnx wheel is not available for this version of python for the " | |||
There was a problem hiding this comment.
It would be friendlier to print the requested Python version here. It would be friendlier yet to state which versions of Python we can support ONNX with.
There was a problem hiding this comment.
Good call! I changed the wording to include the current version of python they are using and a brief summary of what combinations of SS and RAI are supported based on their current platform.
smartsim/_core/_install/buildenv.py
Outdated
| return self.defaults[self.version][name] | ||
| except KeyError: | ||
| raise AttributeError( | ||
| f"'{type(self).__name__}' object has no attribute '{name}'" |
There was a problem hiding this comment.
If I'm trying to install and I get this error message, am I going to have any idea what's going on? I think we need to give the user something to go on here -- even if it's just "contact the dev team"
There was a problem hiding this comment.
Good idea!! Ideally this should never bubble up to a user's view, but on the off chance that it does we should probably give them some direction on where to report it. I added links to out GH issues page and to our community page in our docs
| ] | ||
| # Circumvent a bad `get_deps.sh` script from RAI on 1.2.7 with ONNX | ||
| # TODO: Look for a better way to do this or wait for RAI patch |
There was a problem hiding this comment.
Do we have a ticket open against this workaround?
There was a problem hiding this comment.
| ] | ||
| # Circumvent a bad `get_deps.sh` script from RAI on 1.2.7 with ONNX |
Makes the following changes to SmartSim dependencies: