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.

Sign up for GitHub

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

Jump to bottom

[utils] add preview warning and decorator #26747

Merged
merged 7 commits into from
Jan 3, 2025
Merged

[utils] add preview warning and decorator #26747

merged 7 commits into from
Jan 3, 2025

Conversation

maximearmstrong
Copy link
Contributor

@maximearmstrong maximearmstrong commented Dec 27, 2024 •
edited
Loading

Summary & Motivation

Fixes AD-735 in the API Lifecycle project.

Adding new Preview Python utils. Similar to #25363

This is done in alignment with the new API lifecycle.

The raised warning is a Warning.

Docs to be updated in subsequent PR.

@maximearmstrong Graphite App
Copy link
Contributor Author

maximearmstrong commented Dec 27, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@maximearmstrong maximearmstrong mentioned this pull request Dec 27, 2024
Merged
@maximearmstrong maximearmstrong marked this pull request as ready for review December 27, 2024 21:19
@maximearmstrong maximearmstrong self-assigned this Dec 27, 2024
yuhan
yuhan reviewed Dec 27, 2024
View reviewed changes
python_modules/dagster/dagster/_utils/warnings.py Outdated
return

warnings.warn(
f"{subject} is a preview in early testing phase with frequent changes, not recommended for production use."
Copy link
Contributor

Choose a reason for hiding this comment

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

i know this is the original language we had in the doc. but when it's putting in a user-facing warning like this, it feels a little scary. how about tuning it down:

is a preview currently in early testing, with ongoing updates, and not yet optimized for production use.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "production use" verbiage is okay here, but just swinging back from the upstack review, maybe:

is currently in preview, with ongoing updates. This may break in future versions, even between dot releases.

Copy link
Contributor Author

@maximearmstrong maximearmstrong Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That makes sense. Updated in a2e7976 to

is currently in preview, with ongoing updates, and not yet optimized for production use. This may break in future versions, even between dot releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove "and not yet optimized for production use" if we think it's too long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 9734765 after receiving feedback in this thread

is currently in preview, and may have breaking changes in patch version releases. This feature is not considered ready for production use.

OwenKephart
OwenKephart reviewed Dec 31, 2024
View reviewed changes
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

agreed w/ Yuhan re: the verbiage, otherwise seems good to me

OwenKephart
OwenKephart previously requested changes Dec 31, 2024
View reviewed changes
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

requesting changes for queue management

@maximearmstrong maximearmstrong force-pushed the maxime/ad-735/add-preview-annotation branch from d472b02 to a2e7976 Compare January 2, 2025 15:26
@maximearmstrong maximearmstrong force-pushed the maxime/ad-735/add-preview-annotation branch from 71d62fd to 9734765 Compare January 2, 2025 17:34
@maximearmstrong maximearmstrong requested review from PedramNavid and cmpadden and removed request for cmpadden and danielgafni January 2, 2025 17:36
cmpadden
cmpadden approved these changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@cmpadden cmpadden left a comment

Choose a reason for hiding this comment

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

Messaging looks good given our conversation in Slack.

@maximearmstrong maximearmstrong merged commit e81309e into master Jan 3, 2025
1 check passed
@maximearmstrong maximearmstrong deleted the maxime/ad-735/add-preview-annotation branch January 3, 2025 15:32
This was referenced Jan 3, 2025
Merged
Open
maximearmstrong added a commit that referenced this pull request Jan 9, 2025
@maximearmstrong
[docs] display preview warning and decorator info (#26819)
18fbec5 
## Summary & Motivation

Same as #25362 but for the new preview decorator introduced in
#26747
@maximearmstrong maximearmstrong mentioned this pull request Jan 10, 2025
Merged
maximearmstrong added a commit that referenced this pull request Jan 10, 2025
@maximearmstrong
[docs-beta] Change preview and beta language in API lifecycle guide (#…
e1c7c6e 
…27029)

## Summary & Motivation

As title, following requested changes in this [PR
stack](#26747).

## How I Tested These Changes

Docs preview
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmpadden cmpadden cmpadden approved these changes

@OwenKephart OwenKephart Awaiting requested review from OwenKephart

@yuhan yuhan Awaiting requested review from yuhan

@PedramNavid PedramNavid Awaiting requested review from PedramNavid

Assignees

@maximearmstrong maximearmstrong

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maximearmstrong @yuhan @cmpadden @OwenKephart