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

Start to remove a bunch of complicated 0.63 modal.Cls code #2878

Merged
merged 7 commits into from
Feb 19, 2025

Conversation

freider
Copy link
Contributor

@freider freider commented Feb 18, 2025

Background:

  • For v0.62 lookup compatibility, we used to create _Function objects for every method.
  • When we deprecated v0.62, we still had this code in place even though the class-bound method was no longer backed by any "real" backend function. In the client it mostly served as an information carrier to let us hydrate the (client-only) instance bound _Function.

This PR removes the need for this complicated information carrier, reducing the risk of us accidentally calling the class-bound method.

This will significantly help with further simplification of the class code, and once 0.66 is deprecated we should be able to remove further complicated code as the method metadata will then always be served directly by the class service function rather than the Cls.

TODO:

  • Test that this works with lookups of pre-0.67 classes
  • Run integration tests

Check these boxes or delete any item (or this section) if not relevant for this PR.

  • Client+Server: this change is compatible with old servers
  • Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.


Changelog

@freider freider requested a review from devennavani February 18, 2025 14:44
fun._obj = service_function._obj
fun._is_method = True
fun._app = class_bound_method._app
fun._spec = class_bound_method._spec
fun._is_web_endpoint = class_bound_method._is_web_endpoint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_is_web_endpoint is a method, so this was a weird assignment...

@freider freider requested review from mwaskom and removed request for mwaskom and devennavani February 18, 2025 14:46
@freider freider marked this pull request as draft February 18, 2025 14:55
@freider freider marked this pull request as ready for review February 18, 2025 15:23
@erikbern
Copy link
Contributor

Nice!!

Copy link
Contributor

@devennavani devennavani left a comment

Choose a reason for hiding this comment

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

lgtm!

@freider freider merged commit 4d645ae into main Feb 19, 2025
24 checks passed
@freider freider deleted the freider/remove-method-binding-on-cls branch February 19, 2025 13:14
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.

3 participants