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

More docs for .new deprecation warning #1796

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erikbern
Copy link
Contributor

@erikbern erikbern commented May 6, 2024

Slightly more docs to make it clear that .new -> .ephemeral is not a straightforward substitution

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.

Auto-approved 👍. This diff qualified for automatic approval and doesn't need follow up review.

@erikbern erikbern requested a review from aksh-at May 6, 2024 12:59
@erikbern erikbern force-pushed the erikbern/new-depr-warning-docs branch from f631ce6 to ca47050 Compare May 6, 2024 13:01
Copy link
Contributor

@mwaskom mwaskom left a comment

Choose a reason for hiding this comment

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

See two copy-pasta nits.

IMO this may still be a little confusing since new -> from_name is a nontrivial behavioral change and i'm not sure that "persisted" is a magic-enough word to communicate that.

@@ -87,7 +87,9 @@ class _Queue(_Object, type_prefix="qu"):
def new():
"""`Queue.new` is deprecated.

Please use `Queue.from_name` (for persisted) or `Queue.ephemeral` (for ephemeral) queues.
You can switch this out for `Queue.from_name("some-queue-identifier")` for persisted dicts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can switch this out for `Queue.from_name("some-queue-identifier")` for persisted dicts.
You can switch this out for `Queue.from_name("some-queue-identifier")` for persisted queues.

@@ -139,7 +139,9 @@ def _initialize_from_empty(self):
def new() -> "_Volume":
"""`Volume.new` is deprecated.

Please use `Volume.from_name` (for persisted) or `Volume.ephemeral` (for ephemeral) volumes.
You can switch this out for `Volume.from_name("some-volume-identifier")` for persisted dicts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can switch this out for `Volume.from_name("some-volume-identifier")` for persisted dicts.
You can switch this out for `Volume.from_name("some-volume-identifier")` for persisted volumes.

Comment on lines +143 to +144
You can also take a look at the documentation for `Volume.ephemeral` for another option,
although it might require more code changes if you're coming from `Volume.new`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a hard time finding which docs this refers to. Is it https://modal.com/docs/reference/modal.Volume#ephemeral? Ideally we should add a guide page on "ephemeral objects" and link to that here directly

@mwaskom
Copy link
Contributor

mwaskom commented May 29, 2024

Should we close this now that it's an error?

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.

None yet

3 participants