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

Support functools.partial for event handlers #789

Merged
merged 6 commits into from
Aug 24, 2024
Merged

Conversation

wwwillchen
Copy link
Collaborator

@wwwillchen wwwillchen commented Aug 13, 2024

Fixes #766

@wwwillchen wwwillchen marked this pull request as ready for review August 13, 2024 23:03
@wwwillchen wwwillchen changed the title Support functools.partial (needs test) Support functools.partial for event handlers Aug 13, 2024
@malmaud
Copy link
Contributor

malmaud commented Aug 14, 2024

I'm not a super-expert on the best way to do this but this seems reasonable to me.

@wwwillchen
Copy link
Collaborator Author

@richard-to WDYT about this feature / implementation? I think the feature is useful, but the implementation is a little bit hairy. In particular, we need to avoid getting a repr for types that return a memory address (e.g. vanilla Python classes)

class Car:
  def __init__(self, name: str) -> None:
    self.name = name

car = Car(name="a") # repr -> <__main__.Car object at 0x79cfdc146080>

Copy link
Collaborator

@richard-to richard-to left a comment

Choose a reason for hiding this comment

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

Nice! I remember when I first started using Mesop, I had tried using functools.partial to no avail.

Looking at the code example, it does make the devex a lot nicer than having to pass in a key and then parsing the string.

Question: does this work when doing a dynamic loop? Say I wanted to add an event handler to each box using partial?

Yes, the limitation of needing a stable repr is interesting. How much of a drawback do you think that will be in practice? I think it may be slightly annoying, but the improvement in devex is worth it I think (granted I haven't tested this out with some of the uses I have for this yet)

@wwwillchen
Copy link
Collaborator Author

Nice! I remember when I first started using Mesop, I had tried using functools.partial to no avail.

Looking at the code example, it does make the devex a lot nicer than having to pass in a key and then parsing the string.

Question: does this work when doing a dynamic loop? Say I wanted to add an event handler to each box using partial?

Yup. I added an example in the functools_partial example.

Yes, the limitation of needing a stable repr is interesting. How much of a drawback do you think that will be in practice? I think it may be slightly annoying, but the improvement in devex is worth it I think (granted I haven't tested this out with some of the uses I have for this yet)

I think it's not too bad since this should cover most of the cases. It may be a little hard to understand for Mesop app developers, but I think we can work on improving the error message + docs if this becomes a common issue.

Thanks for the feedback, I'll merge this in.

@wwwillchen wwwillchen merged commit 2dfc770 into google:main Aug 24, 2024
4 checks passed
@wwwillchen wwwillchen deleted the i766 branch August 24, 2024 22:34
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.

Support functools.partial instances as event handlers
3 participants