-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[core-api] base #26770
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
[core-api] base #26770
Conversation
efa7c18
to
28e73c5
Compare
655ae75
to
f2e00af
Compare
f2e00af
to
7a526b2
Compare
#26707) decision: experimental -> beta reason: we might want to subtly change the default behavior of this (e.g. how far down the tree it looks) docs exist: n/a > Insert changelog entry or delete this section.
803c15e
to
7988612
Compare
## Summary & Motivation decision: experimental -> public (uncertain) reason: this has existed for years without change. even if this is not a commonly-used API, we should probably just make the call that we're not changing it at this point docs exist: yes (api only, which is ok) ## How I Tested These Changes ## Changelog > Insert changelog entry or delete this section.
7988612
to
04f8f45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, but LGTM!
@@ -241,7 +241,7 @@ def groups(*group_strs, include_sources: bool = False) -> "GroupsAssetSelection" | |||
|
|||
@public | |||
@staticmethod | |||
@experimental | |||
@experimental_param(param="include_sources") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use @beta_param here?
@whitelist_for_serdes( | ||
old_fields={"time_window_partition_scope_minutes": 1e-6}, | ||
serializer=AutoMaterializePolicySerializer, | ||
) | ||
@deprecated(breaking_version="1.10.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[1] Should we add a message, maybe suggesting DA?
@@ -211,7 +211,7 @@ def evaluate_for_asset(self, context: "AutomationContext") -> "AutomationResult" | |||
|
|||
|
|||
@whitelist_for_serdes | |||
@experimental | |||
@deprecated(breaking_version="1.10.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[1]
…26710) ## Summary & Motivation decision: experimental -> GA (after parameter change) reason: if we're going to make this change, we should acompany it with marking this API as stable (i.e. we should only make the name change once). "source assets" as a concept no longer really exist, and external assets is the more general term (and more accurate to what these parameters were actually filtering for. docs exist: yes (api only, which is ok) ## How I Tested These Changes ## Changelog > Insert changelog entry or delete this section.
This reverts commit 133b1a8.
## Summary & Motivation This is the base of the core-api-audit stack. The entire stack will be merged in at the 1.10 release, and for now to keep things tidy, changes will be merged downwards as they are approved. This means that this PR contains only code that has been reviewed and approved in a different PR. ## How I Tested These Changes ## Changelog The `include_sources` param on all `AssetSelection` APIs has been renamed to `include_external_assets`.
## Summary & Motivation As title. This PR is pending the merged of PR #26770.
## Summary & Motivation As title. This PR is pending the merged of PR #26770.
## Summary & Motivation This is the base of the core-api-audit stack. The entire stack will be merged in at the 1.10 release, and for now to keep things tidy, changes will be merged downwards as they are approved. This means that this PR contains only code that has been reviewed and approved in a different PR. ## How I Tested These Changes ## Changelog The `include_sources` param on all `AssetSelection` APIs has been renamed to `include_external_assets`.
) ## Summary & Motivation As title. This PR is pending the merged of PR dagster-io#26770.
## Summary & Motivation This is the base of the core-api-audit stack. The entire stack will be merged in at the 1.10 release, and for now to keep things tidy, changes will be merged downwards as they are approved. This means that this PR contains only code that has been reviewed and approved in a different PR. ## How I Tested These Changes ## Changelog The `include_sources` param on all `AssetSelection` APIs has been renamed to `include_external_assets`.
## Summary & Motivation As title. This PR is pending the merged of PR #26770.
## Summary & Motivation This is the base of the core-api-audit stack. The entire stack will be merged in at the 1.10 release, and for now to keep things tidy, changes will be merged downwards as they are approved. This means that this PR contains only code that has been reviewed and approved in a different PR. ## How I Tested These Changes ## Changelog The `include_sources` param on all `AssetSelection` APIs has been renamed to `include_external_assets`.
) ## Summary & Motivation As title. This PR is pending the merged of PR dagster-io#26770.
Summary & Motivation
This is the base of the core-api-audit stack. The entire stack will be merged in at the 1.10 release, and for now to keep things tidy, changes will be merged downwards as they are approved.
This means that this PR contains only code that has been reviewed and approved in a different PR.
How I Tested These Changes
Changelog
The
include_sources
param on allAssetSelection
APIs has been renamed toinclude_external_assets
.