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

Jump to bottom

Smart Info #350

Merged
merged 4 commits into from
Aug 25, 2023
Merged

Smart Info #350

merged 4 commits into from
Aug 25, 2023

Conversation

MattToast
Copy link
Member

@MattToast MattToast commented Aug 25, 2023 •
edited
Loading

Add a smart info target for info about current SS installation

Example Output:

$ smart info

Smart Python Packages:
╒════════════╤═══════════╕
│ Name       │ Version   │
╞════════════╪═══════════╡
│ SmartSim   │ 0.5.0     │
│ SmartRedis │ 0.4.1     │
╘════════════╧═══════════╛

Orchestrator Configuration:
╒═══════════╤════════════════════════════════════════════════════════════╕
│ Installed │ REDIS                                                      │
│ Location  │ /Users/mrdro/repos/ssimdev/ss/smartsim/_core/bin/redis-cli │
╘═══════════╧════════════════════════════════════════════════════════════╛

Redis AI Configuration:
╒══════════════╤═════════════════════════════════════════════════════════════╕
│ Is Installed │ Installed                                                   │
│ Location     │ /Users/mrdro/repos/ssimdev/ss/smartsim/_core/lib/redisai.so │
╘══════════════╧═════════════════════════════════════════════════════════════╛

Machine Learning Backends:
╒════════════╤═════════════════════╤══════════════════╕
│ Name       │ Backend Available   │ Python Package   │
╞════════════╪═════════════════════╪══════════════════╡
│ Tensorflow │ True                │ 2.8.0            │
│ Torch      │ True                │ 1.11.0           │
│ ONNX       │ False               │ Not Installed    │
╘════════════╧═════════════════════╧══════════════════╛

@MattToast MattToast added the type: usability Issues related to ease of use label Aug 25, 2023
@MattToast MattToast requested a review from al-rigazzi August 25, 2023 18:44
@MattToast MattToast self-assigned this Aug 25, 2023
@codecov
Copy link

codecov bot commented Aug 25, 2023 •
edited
Loading

Codecov Report

Merging #350 (d1b21f2) into develop (d7a6b60) will increase coverage by 0.16%.
Report is 2 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #350      +/-   ##
===========================================
+ Coverage    87.04%   87.21%   +0.16%     
===========================================
  Files           59       59              
  Lines         3551     3551              
===========================================
+ Hits          3091     3097       +6     
+ Misses         460      454       -6

see 2 files with indirect coverage changes

al-rigazzi
al-rigazzi requested changes Aug 25, 2023
View reviewed changes
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

Just one thing to think about and then looks great!

smartsim/_core/_cli/info.py Outdated

print("Redis AI Configuration:")
rai_path = _helpers.redis_install_base().parent / "redisai.so"
rai_table = [["Is Installed", _fmt_installed_redis_ai(rai_path)]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sure you thought about this for a lot (isn't this the kind of things we spend most of the dev time?) but I don't love the redundancy in Is installed | Installed - what about one of the following two:

  • Is installed | Yes/No
  • Status | Installed/Not installed

And is there any chance we can also output the Redis AI version?

Copy link
Member Author

Choose a reason for hiding this comment

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

LOL, I also didn't love the redundancy! Changing to Status!

Copy link
Member Author

@MattToast MattToast Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't find an immediate way to get the RAI version without saving off some persistent artifact from the build process or without spinning up Redis Server, both of which seemed like a lot of work to try and and cram into a "quick and dirty" implementation for this release.

This is absolutely something that I was planning on tackling when I implement this feature "properly" in a follow on PR next sprint!

@MattToast MattToast requested a review from al-rigazzi August 25, 2023 20:02
al-rigazzi
al-rigazzi approved these changes Aug 25, 2023
View reviewed changes
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

LGTM!

@MattToast MattToast merged commit 90ba36e into CrayLabs:develop Aug 25, 2023
@Spartee
Copy link
Contributor

Spartee commented Aug 27, 2023

@MattToast @al-rigazzi This is sexy.

@MattToast MattToast deleted the smart-info branch September 11, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@al-rigazzi al-rigazzi al-rigazzi approved these changes

Assignees

@MattToast MattToast

Labels
type: usability Issues related to ease of use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MattToast @Spartee @al-rigazzi