Skip to content

[redis-py] relax redis-py dep and update redis-py h{m}set calls#11776

Merged
rkooo567 merged 1 commit intoray-project:masterfrom
acxz:redis-update
Nov 4, 2020
Merged

[redis-py] relax redis-py dep and update redis-py h{m}set calls#11776
rkooo567 merged 1 commit intoray-project:masterfrom
acxz:redis-update

Conversation

@acxz
Copy link
Copy Markdown
Contributor

@acxz acxz commented Nov 3, 2020 •
edited
Loading

Why are these changes needed?

Relaxing the redis dep is needed as the Arch Linux package is already on redis==3.5.3 and ray will not run otherwise. This PR updates redis api so that we can safely unpin the dep.

Related issue number

Closes #8257
Closes #9069
Closes #11632
Downstream issue: acxz/pkgbuilds#80

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(
      N/A as this is a build PR.

@acxz acxz mentioned this pull request Nov 3, 2020
Closed
@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Nov 3, 2020

I think the original reason why we had this constraint was one of Redis APIs were producing tons of warnings after 3.5.0. I remember it was hmset or something. Do you mind replacing those APIs to hset?

@rkooo567 rkooo567 self-assigned this Nov 3, 2020
@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Nov 3, 2020

Related; #8257

@acxz
Copy link
Copy Markdown
Contributor Author

acxz commented Nov 3, 2020

Thanks for pointing me to the issue, yeah I can take a look at it.

@acxz
Copy link
Copy Markdown
Contributor Author

acxz commented Nov 3, 2020 •
edited
Loading

@rkooo567 I went ahead and changed our internal java redis api for hmset to hset to ensure matching names. Let me know if you think I should revert that commit. Nvm think it's better to change the java api call in a different PR. This should just focus on redis-py.

The windows ci seems to fail for an unrelated reason.

@kfstorm
Copy link
Copy Markdown
Member

kfstorm commented Nov 3, 2020 •
edited
Loading

By changing hmset to hset, we now requires redis >= 4.0.0, right? According to https://redis.io/commands/hset, The minimum redis version that HSET supports multiple fields/values is 4.0.0.

@acxz
Copy link
Copy Markdown
Contributor Author

acxz commented Nov 3, 2020 •
edited
Loading

That is a good observation, but redis and redis-py operate on two different versioning schemes. Your comment does bring to light that I should bump up the lower bound to 3.5.0, however.

@kfstorm kfstorm mentioned this pull request Nov 3, 2020
Closed
6 tasks
@acxz acxz changed the title relax redis dep [redis-py] relax redis-py dep and update redis-py h{m}set calls Nov 4, 2020
@rkooo567
Copy link
Copy Markdown
Contributor

rkooo567 commented Nov 4, 2020

Before merging, I'd like to have a sanity check from @simon-mo. Do you see any concern to bump up the minimum Redis version in our repo?

@simon-mo
Copy link
Copy Markdown
Contributor

simon-mo commented Nov 4, 2020

LGTM

@rkooo567 rkooo567 merged commit b7531fb into ray-project:master Nov 4, 2020
@acxz acxz deleted the redis-update branch November 4, 2020 12:26
@simon-mo simon-mo mentioned this pull request Dec 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rkooo567 rkooo567 rkooo567 approved these changes

Assignees

@rkooo567 rkooo567

@simon-mo simon-mo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ray] Make Ray compatible with latest redis Redis-Py Deprecation Warnings

4 participants

@acxz @rkooo567 @kfstorm @simon-mo