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.

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

Keydb support #180

Merged
merged 7 commits into from
Apr 11, 2022
Merged

Keydb support #180

merged 7 commits into from
Apr 11, 2022

Conversation

EricGustin
Copy link
Contributor

@EricGustin EricGustin commented Mar 30, 2022

This PR adds the ability to use KeyDB in place of Redis as the database through a new --keydb flag for smart build. This flexibility enables users to maximize their throughput if they desire.

REDIS_CONF, SMARTSIM_REDIS, SMARTSIM_REDIS_URL, SMARTSIM_REDIS_BRANCH need to be set for KeyDB
@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2022

Codecov Report

Merging #180 (c660dae) into develop (3efd2a0) will increase coverage by 0.46%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #180      +/-   ##
===========================================
+ Coverage    81.20%   81.66%   +0.46%     
===========================================
  Files           57       57              
  Lines         2910     2973      +63     
===========================================
+ Hits          2363     2428      +65     
+ Misses         547      545       -2     
Impacted Files Coverage Δ
smartsim/_core/launcher/colocated.py 85.71% <ø> (ø)
smartsim/_core/utils/redis.py 100.00% <ø> (ø)
smartsim/_core/config/config.py 95.83% <100.00%> (ø)
smartsim/database/orchestrator.py 83.93% <100.00%> (ø)
smartsim/_core/generation/modelwriter.py 84.93% <0.00%> (-4.78%) ⬇️
smartsim/experiment.py 80.98% <0.00%> (ø)
smartsim/_core/control/jobmanager.py 94.03% <0.00%> (+1.57%) ⬆️
smartsim/_core/control/controller.py 82.71% <0.00%> (+1.74%) ⬆️
smartsim/settings/base.py 93.57% <0.00%> (+2.26%) ⬆️
smartsim/ml/tf/utils.py 95.83% <0.00%> (+2.50%) ⬆️

@EricGustin EricGustin requested review from Spartee, al-rigazzi and MattToast and removed request for Spartee and al-rigazzi April 1, 2022 17:52
@EricGustin
Copy link
Contributor Author

EricGustin commented Apr 1, 2022

Should we add tests for this code by using a new github actions job that does smart build with varying flags?

@EricGustin EricGustin marked this pull request as ready for review April 1, 2022 17:57
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

lgtm!!

Everything seems to work on my end, only change I'd request is adding smartsim/_core/bin/keydb-server and smartsim/_core/bin/keydb-cli to the .gitignore

@MattToast
Copy link
Member

Should we add tests for this code by using a new github actions job that does smart build with varying flags?

I think that seems like a great idea! If not on this PR maybe in a new ticket?

Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Nice piece of work and it dovetails well with some of the things I'm trying to do. Main comment is to improve the safety of the code by removing the wildcards when removing files.

Looking forward to having this functionality in.

smartsim/_core/_cli/cli.py Outdated Show resolved Hide resolved
smartsim/_core/_install/builder.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/clean.py Show resolved Hide resolved
smartsim/_core/_cli/build.py Show resolved Hide resolved
Spartee
Spartee previously requested changes Apr 6, 2022
smartsim/_core/_cli/build.py Show resolved Hide resolved
smartsim/_core/_cli/clean.py Show resolved Hide resolved
smartsim/_core/_cli/cli.py Outdated Show resolved Hide resolved
smartsim/_core/_install/buildenv.py Outdated Show resolved Hide resolved
smartsim/_core/_install/builder.py Outdated Show resolved Hide resolved
smartsim/_core/_cli/build.py Show resolved Hide resolved
@EricGustin EricGustin dismissed Spartee’s stale review April 11, 2022 23:10

Review has been addressed & was given the green light via Slack

@EricGustin EricGustin merged commit 7c7978f into CrayLabs:develop Apr 11, 2022
@EricGustin EricGustin deleted the keydb-support branch April 11, 2022 23:12
al-rigazzi pushed a commit to al-rigazzi/SmartSim that referenced this pull request Apr 18, 2022
keydb flag for smart build added. Users can now
use KeyDB in place of Redis more easily.

[ committed by @EricGustin ]
[ reviewed by @MattToast @ashao @Spartee ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants