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

Expose MINBATCHTIMEOUT parameter in set_model interface #406

Merged
merged 10 commits into from Oct 4, 2023

Conversation

billschereriii
Copy link
Contributor

No description provided.

@billschereriii billschereriii added type: feature Issues that include feature request or feature idea area: python Issues related to the Python Client area: C++ Issues related to the C++ client area: C Issues related to the C client area: fortran issues related to the Fortran Client API break Issues that include incompatible API changes labels Oct 2, 2023
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #406 (f46b1c4) into develop (53def75) will increase coverage by 0.18%.
The diff coverage is 57.89%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #406      +/-   ##
===========================================
+ Coverage    78.19%   78.38%   +0.18%     
===========================================
  Files           97       97              
  Lines         7178     7204      +26     
===========================================
+ Hits          5613     5647      +34     
+ Misses        1565     1557       -8     
Files Coverage Δ
include/client.h 100.00% <ø> (ø)
include/redisserver.h 60.00% <ø> (ø)
src/c/c_client.cpp 56.21% <ø> (ø)
src/cpp/client.cpp 88.66% <100.00%> (+0.59%) ⬆️
src/python/module/smartredis/client.py 96.49% <100.00%> (+0.74%) ⬆️
src/python/src/pyclient.cpp 77.65% <ø> (+1.11%) ⬆️
src/cpp/redis.cpp 53.18% <25.00%> (+0.47%) ⬆️
src/cpp/rediscluster.cpp 66.52% <25.00%> (+0.22%) ⬆️
src/fortran/client.F90 47.90% <33.33%> (-0.13%) ⬇️

... and 1 file with indirect coverage changes

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.

Thanks for turning that around so quick! Will definitely lead to easy performance gains.

Only one request from me

  • Throw a warning if MINBATCHSIZE>0 and MINBATCHTIMEOUT==0 to nudge the user. Something along the lines of min_batch_timeout was set with a non-zero min_batch_size. Performance may improve by passing in a small value (~10ms) for min_batch_timeout
  • Throw an exception if MINBATCHTIMEOUT>0 and (MINBATCHSIZE==0 or BATCHSIZE==0) with the message that min_batch_size and batch_size must be non-zero if min_batch_timeout>0

Add those tests to ensure we're throwing those to test_errors.py.

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.

Really appreciate the new usages exceptions/warnings. Also nice work on adding the test to capture the warning

@@ -67,6 +67,13 @@ jobs:
GCC_V: ${{ matrix.compiler }} # used when the compiler is gcc/gfortran

steps:
# Free up some disk space
run: |
Copy link
Member

Choose a reason for hiding this comment

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

Ooof really these are the kind of games that we have to play?Q

@billschereriii billschereriii merged commit 2fe3228 into CrayLabs:develop Oct 4, 2023
26 checks passed
@billschereriii billschereriii deleted the set_muddle branch October 4, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API break Issues that include incompatible API changes area: C++ Issues related to the C++ client area: C Issues related to the C client area: fortran issues related to the Fortran Client area: python Issues related to the Python Client type: feature Issues that include feature request or feature idea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants