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

Migration from redis to valkey #12618

Closed
Rafiot opened this issue Sep 5, 2024 · 4 comments
Closed

Migration from redis to valkey #12618

Rafiot opened this issue Sep 5, 2024 · 4 comments

Comments

@Rafiot
Copy link
Contributor

Rafiot commented Sep 5, 2024

As you may know, redis cannot really be considered opensource anymore, and pretty much everything and everyone moved to valkey and that includes the python module, now called valkey-py.

The other issue is that the typing provided by redis-py/valkey-py does not work at all, for anyone: valkey-io/valkey-py#84 (comment), which is why everyone is using types-redis, but that's not an option with valkey-py anymore.

The plan is to avoid requiring the types-redis all together (which is good, as it is flagged for removal), but this is going to require some work (valkey-io/valkey-py#84).

Do you have any recommandations on how to proceed? The package is a bit tricky (see the issue again), and I think the proper way to do it will be to copy the pyi files in the valkey-py repo, do the renaming, and work from there. But do you have any recommandations on how to proceed?

@srittau
Copy link
Collaborator

srittau commented Sep 5, 2024

Thanks for working on valkey! In my experience working on the redis stubs, typing for it was quite hard, since it uses some API patterns that didn't mix well with typing. So I understand the challenges valkey-py faces.

I basically see two possible avenues:

  • Improve the typing support in valkey-py itself. This is the most user friendly approach, as users don't need to install and update/keep in sync an additional package. It should work out of the box for them. It also has the advantage that keeping types and implementation in sync happens "automatically". In general, this is the approach recommended by typeshed maintainers, if the upstream maintainers want to support typing. The Type School was an attempt to allow people to ask for reviews of type annotation PRs in their packages by type-experienced developers. While it didn't take off, it's still available.
  • Add a types-valkey package to typeshed (probably based on the current types-redis package). This has the advantage that typeshed has superb tooling for stubs and experienced maintainers that can help with typing. But it has a worse user experience and this depends on the workload of typeshed maintainers. (We have a bit of a backlog at the moment, although our backlog works more like a stack than a heap, so PR authors can get lucky.)

There's also a hybrid approach, which uses typeshed as an incubator. Start with stubs in typeshed, and once they are in a good shape, transfer the annotations over to the upstream package. Maintaining high quality annotations is easier than creating them in the first place.

@Avasam
Copy link
Collaborator

Avasam commented Sep 5, 2024

  • Add a types-valkey package to typeshed (probably based on the current types-redis package). This has the advantage that typeshed has superb tooling for stubs and experienced maintainers that can help with typing. But it has a worse user experience and this depends on the workload of typeshed maintainers. (We have a bit of a backlog at the moment, although our backlog works more like a stack than a heap, so PR authors can get lucky.)

There's also a hybrid approach, which uses typeshed as an incubator. Start with stubs in typeshed, and once they are in a good shape, transfer the annotations over to the upstream package. Maintaining high quality annotations is easier than creating them in the first place.

+1 it's essentially what I'm doing with pywin32 and how I'm gradually working on moving types-setuptools upstream.

I'd like to add that if the valkey maintainer(s) themselve(s) contribute to typeshed stubs, it's a lot easier to know what was the intent behind certain type restrictions, focusing more on type completion and correctness, maybe even bringing fixes upstream (I've had types-redis contributions blocked by incorrect Protocol usage upstream with no clean way of fixing in typeshed).

I'm also always a bit wary of adding stubs that could've been proposed upstream in the first place, but I don't have any issue using typeshed as an "incubator" or "proving grounds"

That being said, it would also mean that typing in Valkey would be subject to the whims of typeshed maintainers' availability.


For your linked async issue specifically, I don't see a great way to fix it without:

  • erasing the type (Any, you can always alias it so that it makes sense to a human reader);
  • reworking everything to split async and sync methods;
  • possibly a mypy plugin;
  • for each return type, creating a type-only class that subclasses the original and adds the async methods. Something like:
    # Not clean, and needs to be done for every super-class, but could give your your "join-like" type structure
    # This is essentially how we currently join protocols.
    
    # The type_check_only decorator only works in `.pyi` files,
    # but I think hiding these in a stub file and not exposing them at runtime might be the best call if going that route
    
    @type_check_only  
    class MaybeAsyncDict(dict[_KT, _KV]):
        def __await__(self) -> dict[_KT, _KV]: ...
  • or a proposal like AnyOf - Union for return types typing#566

(see python/mypy#1693)

@aiven-sal
Copy link

On behalf of valkey-py's maintainers I can say that we certainly want the typing to be part of the project (now or at some point later).
So, if you want to use typeshed as an incubator it sounds good for me, but if you think that the work should be done upstream, it's also fine.

@srittau
Copy link
Collaborator

srittau commented Oct 1, 2024

I'm going to close this now. The possible solutions have been discussed. If there are anymore questions, please let us know.

@srittau srittau closed this as completed Oct 1, 2024
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

No branches or pull requests

4 participants