-
Notifications
You must be signed in to change notification settings - Fork 37
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
Dependency Updates #256
Dependency Updates #256
Changes from 17 commits
27021c6
f1280a9
7677e31
efb1f82
05a5ea5
a68f74a
dc8cbf5
0850315
896a6e0
4800ae0
7ced3bd
fb72bd9
cfa6a19
2bd77b7
b116493
30cc1f7
0eb4e15
f719bed
519814d
0263ccf
03ded5b
16b95a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,9 +16,9 @@ contact_email = [email protected] | |
license = BSD 2-Clause License | ||
keywords = scientific, ai, workflow, hpc, analysis | ||
classifiers = | ||
Programming Language :: Python :: 3.7 | ||
Programming Language :: Python :: 3.8 | ||
Programming Language :: Python :: 3.9 | ||
Programming Language :: Python :: 3.10 | ||
License :: OSI Approved :: BSD License | ||
Intended Audience :: Science/Research | ||
Topic :: Scientific/Engineering | ||
|
@@ -29,7 +29,7 @@ setup_requires = | |
setuptools>=39.2 | ||
cmake>=3.13 | ||
include_package_data = True | ||
python_requires = >=3.7 | ||
python_requires = >=3.8,<3.11 | ||
|
||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,11 +152,6 @@ def __init__(self): | |
# REDIS/KeyDB | ||
self.build_database() | ||
|
||
if self.verbose: | ||
logger.info("Version Information:") | ||
vers = self.versions.as_dict() | ||
print(tabulate(vers, headers=vers.keys(), tablefmt="github"), "\n") | ||
|
||
# REDISAI | ||
self.build_redis_ai( | ||
str(args.device), | ||
|
@@ -201,23 +196,17 @@ def build_redis_ai( | |
): | ||
|
||
# make sure user isn't trying to do something silly on MacOS | ||
if self.build_env.PLATFORM == "darwin": | ||
if device == "gpu": | ||
logger.error("SmartSim does not support GPU on MacOS") | ||
sys.exit(1) | ||
if onnx and self.versions.REDISAI < "1.2.6": | ||
logger.error("RedisAI < 1.2.6 does not support ONNX on MacOS") | ||
sys.exit(1) | ||
if self.versions.REDISAI == "1.2.4" or self.versions.REDISAI == "1.2.5": | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still support RAI 1.2.5, so some of these checks are still needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Now when 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.7 But the same scenario will work on linux with Python<=3.9 $ export SMARTSIM_REDISAI=1.2.5
$ smart build
# Shortened for brevity
$ echo $?
0 Do 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds reasonable to me, but let's bring it up in standup tomorrow and make sure the whole team's onboard |
||
|
||
# decide which runtimes to build | ||
print("\nML Backends Requested") | ||
print("-----------------------") | ||
print(f" PyTorch {self.versions.TORCH}: {color_bool(torch)}") | ||
print(f" TensorFlow {self.versions.TENSORFLOW}: {color_bool(tf)}") | ||
print(f" ONNX {self.versions.ONNX}: {color_bool(onnx)}\n") | ||
backends_table = [ | ||
["PyTorch", self.versions.TORCH, color_bool(torch)], | ||
["TensorFlow", self.versions.TENSORFLOW, color_bool(tf)], | ||
["ONNX", self.versions.ONNX or "Unavailable", color_bool(onnx)], | ||
] | ||
print(tabulate(backends_table, tablefmt="fancy_outline"), end="\n\n") | ||
print(f"Building for GPU support: {color_bool(device == 'gpu')}\n") | ||
|
||
self.check_backends_install() | ||
|
@@ -231,7 +220,6 @@ def build_redis_ai( | |
if tf: | ||
self.check_tf_install() | ||
|
||
cmd = [] | ||
# TORCH | ||
if torch: | ||
if torch_dir: | ||
|
@@ -341,6 +329,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
"requested onnx runtime" | ||
) | ||
try: | ||
if not self.build_env.check_installed("onnx", self.versions.ONNX): | ||
msg = ( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like useful debug info -- why did we take it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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