-
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
Upgrade Machine Learning Dependencies #451
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #451 +/- ##
===========================================
+ Coverage 90.42% 90.47% +0.05%
===========================================
Files 60 60
Lines 3738 3738
===========================================
+ Hits 3380 3382 +2
+ Misses 358 356 -2 |
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.
Thanks for doing this and for cleaning up the builder code. A few suggestions/minor corrections
return vers | ||
ONNX = Version_(REDISAI.onnx) | ||
|
||
def as_dict(self, db_name: DbEngine = "REDIS") -> t.Dict[str, t.Tuple[str, ...]]: |
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.
Definitely out of scope, but you touched, so you get to discuss it with me :)
What do you think about refactoring the name db_name
to db_backend
. To me it seems a bit more intuitive that it refers to a specific implementation of the backend (e.g. vanilla Redis, KeyDB, ToastDB, etc.)
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.
I would LOVE that! I would go so far as as to say we should try remove the parameter entirely. Or at least unify how we collect information in this method.
E.g Why do we need to pass in the DB backed name here, but we ASSUME the self.REDIS
attr will be a str to the correct URL for redis or keydb?
smartsim/_core/_install/builder.py
Outdated
raise BuildError(f"Unrecognized or unsupported operating system '{os_}'") | ||
|
||
machine = platform.machine().lower() | ||
if machine not in ("amd64", "x86_64") and any( |
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.
At some point, maybe in this PR could we start pulling together some of these magic things into its own structure? i.e.
@dataclass
class PlatformRequirements(Requirements):
allowed_architectures = ("amd64", "x86_64")
allowed_systems = ("darwin", "linux")
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.
Totally doable in this PR! I would object to a dataclass
though as I'm not sure what it would mean to "instance" requirements. It seems to me we just want these in their own namespace. Maybe a number of Enum
s?
class AllowedOperatingSystem(enum.Enum):
LINUX = ("linux", "linux2")
DARWIN = ("darwin",)
@classmethod
def from_str(cls, string: str) -> "AllowedOperatingSystem":
for enum_ in cls:
if string in enum_.value:
return enum_
raise BuildError(f"Unrecognized or unsupported OS: {string}")
# Etc for architecture (and device?)
?
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.
That works for me. Let's restrict to OS and architecture for now. Also, I'm kind of vaguely wondering if htey actually need to be in the same class, since there's a potential future with Linux on arm64 or some weird combination that we might want to explicitly disallow
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.
That makes sense! I'll drop the "Supported
/Allowed
" wording as I think that that gives me the same anxiety as putting them in the same class and just make Architecture
and OperatingSystem
enums that we can use as types in place of the t.Literal
strings that we are using now!
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.
Looks good to me, pending the refactor into the "Supported OS/Arch" class. Thanks for these changes and cleanup!
Adds infrastructure to fetch RedisAI's dependencies. This removes the need to call RedisAI's
get_deps.sh
script so that we can fetch newer versions of our machine learning backends than the ones officially supported by RedisAI.Additionally, this upgrades the machine learning python packages required by SmartSim so that they stay up to date with the backends. This in turn allows us to add Python3.10+ONNX support.