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

PoC: get rid of hydrate_lazily concept #2770

Closed
wants to merge 1 commit into from

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented Jan 16, 2025

In theory, all Modal objects now support lazy / just-in-time hydration. So we can simplify the codebase a bit by getting rid of the hydrate_lazily parameter in the Object initialization process, along with some validation logic.

It looks like were weren't passing hydrate_lazily=True everywhere we could, but I can't follow the logic to understand whether that's because those routes truly don't support lazy hydration.

As a good empiricist, my first step is just to delete it and see if the tests pass.

@@ -123,24 +120,10 @@ def _get_metadata(self) -> Optional[Message]:
# the object_id is already provided by other means
return

def _validate_is_hydrated(self: O):
Copy link
Contributor

Choose a reason for hiding this comment

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

good riddance, this code is a jank parade

@freider
Copy link
Contributor

freider commented Jan 17, 2025

I think the one exception to everything being lazy is "local" Functions and Classes, since those require an App to be created to be hydrated. With modal run this is usually not an issue (unless you call functions on another local app), but if you have your own script's and call function.remote() outside of a with app.run() scope there needs to be some kind of error?

@mwaskom
Copy link
Contributor Author

mwaskom commented Jan 17, 2025

Yeah the tests helpfully surfaced some additional complexity here. Need to think about it a little more but will follow up on Slack.

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