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

Handle new XAUTOCLAIM response introduced in redis>=4.3.4 #672

Conversation

andreasjansson
Copy link
Member

A breaking change in redis==4.3.4 breaks Cog.

This PR handles the new XAUTOCLAIM response format in a backwards-compatible way.

Closes #671

[A breaking change](redis/redis-py#2252) in `redis==4.3.4` breaks Cog.

This PR handles the new XAUTOCLAIM response format in a backwards-compatible way.

Closes replicate#671

Signed-off-by: andreasjansson <andreas@replicate.ai>
@andreasjansson andreasjansson force-pushed the redis-4.3.4-xautoclaim-breaking-change branch from 1a614a6 to 1a5b375 Compare June 28, 2022 20:01
@andreasjansson andreasjansson requested a review from zeke June 28, 2022 20:08
@bfirsh
Copy link
Member

bfirsh commented Jun 28, 2022

Looks good. Perhaps we should also pin redis in setup.py to avoid this issue in the future? It seems unlikely that redis will be installed for any other reason.

The real fix: #409

@bfirsh
Copy link
Member

bfirsh commented Jun 28, 2022

This breaks all existing versions of Cog doesn't it? I guess this is a good reason to use #605 :)

@andreasjansson
Copy link
Member Author

Perhaps we should also pin redis in setup.py to avoid this issue in the future?

I decided against this since redis-py is used by a lot of other packages.

But yeah, this is another reminder that we really should vendor dependencies.

@bfirsh
Copy link
Member

bfirsh commented Jun 28, 2022

I wonder if they are going to be used in ML models though?

@andreasjansson
Copy link
Member Author

I wonder if they are going to be used in ML models though?

Ray, Optuna, etc.

It's probably as likely that any of the other packages we depend on introduce breaking changes. I think we should just get this issue fixed and then move to vendor our dependencies.

@bfirsh
Copy link
Member

bfirsh commented Jun 28, 2022

100% all looks good.

@andreasjansson andreasjansson merged commit 9b6bb20 into replicate:main Jun 28, 2022
@andreasjansson andreasjansson deleted the redis-4.3.4-xautoclaim-breaking-change branch June 28, 2022 20:48
bfirsh added a commit that referenced this pull request Jun 28, 2022
I made the update check display a message for #672 but the message
doesn't reset when people upgrade.

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit that referenced this pull request Jun 28, 2022
I made the update check display a message for #672 but the message
doesn't reset when people upgrade.

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
bfirsh added a commit that referenced this pull request Jun 28, 2022
I made the update check display a message for #672 but the message
doesn't reset when people upgrade.

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
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.

Redis issue causing CI to fail
2 participants