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

Allow strings in Python interface for Client.run_script, Client.run_script_multiGPU #407

Merged
merged 13 commits into from
Oct 6, 2023
Merged

Allow strings in Python interface for Client.run_script, Client.run_script_multiGPU #407

merged 13 commits into from
Oct 6, 2023

Conversation

amandarichardsonn
Copy link
Contributor

This PR merges the following to develop

  1. python client.run_script - type hints, type checks and docs
  2. python client.run_script_multiGPU - type hints, type checks and docs
  3. updated changelog

billschereriii
billschereriii suggested changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@billschereriii billschereriii left a comment

Choose a reason for hiding this comment

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

I think maybe you misunderstood the point of this PR. The idea was to add support for single strings while maintaining existing support for lists. In most places, it looks like you've deleted support for lists.

billschereriii
billschereriii suggested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@billschereriii billschereriii left a comment

Choose a reason for hiding this comment

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

Definite improvement, but a few more things to sort out and then this can be merged

billschereriii
billschereriii reviewed Oct 5, 2023
View reviewed changes
doc/changelog.rst Outdated Show resolved Hide resolved
billschereriii
billschereriii suggested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@billschereriii billschereriii left a comment

Choose a reason for hiding this comment

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

A few more tweaks and we're good to go

@codecov
Copy link

codecov bot commented Oct 6, 2023 •
edited
Loading

Codecov Report

Merging #407 (db2347a) into develop (90f6c87) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #407      +/-   ##
===========================================
+ Coverage    78.38%   78.43%   +0.04%     
===========================================
  Files           97       97              
  Lines         7204     7200       -4     
===========================================
  Hits          5647     5647              
+ Misses        1557     1553       -4
Files Coverage Δ
src/python/module/smartredis/client.py 96.93% <ø> (+0.43%) ⬆️

... and 1 file with indirect coverage changes

billschereriii
billschereriii approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@billschereriii billschereriii 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 all your hard work on this one!

@amandarichardsonn
Copy link
Contributor Author

Allow strings in python interface for run_script (#363 )

This PR merges the following to develop:
python client.run_script - type hints, type checks and docs
python client.run_script_multiGPU - type hints, type checks and docs, and
updated changelog.

[ committed by @amandarichardsonn ]
[ reviewed by @billschereriii ]

@amandarichardsonn amandarichardsonn merged commit 0972ec4 into develop Oct 6, 2023
30 checks passed
@amandarichardsonn amandarichardsonn deleted the sr363 branch October 6, 2023 23:10
ashao pushed a commit that referenced this pull request Oct 17, 2023
@amandarichardsonn @ashao
Allow strings in Python run_script methods (#407)
dd5e8fe 
Previously, users could only pass lists of strings into the
run_script methods in Python. This was inconvenient because many
users have scripts that only have single inputs and outputs.
Users are now able to pass in a single string to these methods.

[ committed by @amandarichardsonn ]
[ reviewed by @billschereriii ]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billschereriii billschereriii billschereriii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amandarichardsonn @billschereriii