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

Expose Object.hydrate as public API / deprecate Object.resolve #2769

Merged
merged 7 commits into from
Jan 22, 2025

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented Jan 16, 2025

Describe your changes

We are deprecating the Object.lookup method, which eagerly hydrates the object upon construction, in favor of the following pattern:

  • Most applications can use the lazy Object.from_name and rely on operations triggering on-demand hydration
  • In rare cases where users need to force hydration without performing some other operation, they can call the new Object.hydrate() method.

The .hydrate method replaces the existing .resolve method. The new name was introduced because we have more commonly used "hydration" terminology (i.e. in error messages) and it is more accurate given the current Modal object model. The new method is marginally more user-friendly, e.g. it returns self so it can be called fluently.

Part of CLI-96

Backward/forward compatibility checks

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

  • Introduced a new public method, .hydrate, for on-demand hydration of Modal objects. This method replaces the existing semi-public .resolve method, which is now deprecated.

@mwaskom mwaskom requested a review from freider January 16, 2025 22:47
@mwaskom mwaskom closed this Jan 16, 2025
@mwaskom mwaskom reopened this Jan 17, 2025
@mwaskom
Copy link
Contributor Author

mwaskom commented Jan 17, 2025

Whoops meant to abandon #2770, not this one!

@mwaskom mwaskom force-pushed the michael/2025-01-15-object-hydrate-method branch from 8899c10 to 495f365 Compare January 17, 2025 17:31
Copy link
Contributor

@erikbern erikbern left a comment

Choose a reason for hiding this comment

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

👌

@mwaskom mwaskom force-pushed the michael/2025-01-15-object-hydrate-method branch from 493ff48 to ab3cb55 Compare January 21, 2025 13:37
@mwaskom mwaskom force-pushed the michael/2025-01-15-object-hydrate-method branch from ab3cb55 to 8051d9c Compare January 22, 2025 13:45
@mwaskom
Copy link
Contributor Author

mwaskom commented Jan 22, 2025

@prbot approve

Copy link

@modal-pr-review-automation modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

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

Approved 👍. @freider will follow-up review this.

@@ -517,8 +517,9 @@ async def exec(
raise InvalidError(f"workdir must be an absolute path, got: {workdir}")

# Force secret resolution so we can pass the secret IDs to the backend.
# TODO should we parallelize this?
Copy link
Contributor

@freider freider Jan 22, 2025

Choose a reason for hiding this comment

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

Yes! cries in eu latencies

elif not self._hydrate_lazily:
# TODO(michael) can remove _hydrate lazily? I think all objects support it now?
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything except locally defined Functions and Classes (as defined by the function and cls decorators)

Copy link
Contributor

@freider freider left a comment

Choose a reason for hiding this comment

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

lgtm

@mwaskom mwaskom merged commit 12904f5 into main Jan 22, 2025
23 checks passed
@mwaskom mwaskom deleted the michael/2025-01-15-object-hydrate-method branch January 22, 2025 14:36
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