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

Extend visit_collection with support for annotations #7263

Merged
merged 15 commits into from
Jan 6, 2023
Merged

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Oct 20, 2022

Extends #7248 but solves PrefectHQ/prefect-dask#43 without requiring the changes in PrefectHQ/prefect-dask#45

There is a new context dictionary on visit_collection. It is None by default and ignored. If set, it can be used to store contextual information that persists as visit_collection descends into nested items. A shallow copy is used to prevent the changes from leaking "up".

This context dictionary is populated with an annotation when descending into annotations. Previously, visit_collection would never descend into annotations. We now explicitly do not parse object within quotes in the engine instead of relying on visit_collection to ignore it.

Additionally, visit_collection now has a remove_annotations: bool = False setting that is used during resolve_inputs to strip annotations from inputs while preserving other behavior i.e. visiting the items within the annotation.

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a label categorizing the change e.g. fix, feature, enhancement

@netlify
Copy link

netlify bot commented Oct 20, 2022

Deploy Preview for prefect-orion ready!

Name Link
🔨 Latest commit c6e725d
🔍 Latest deploy log https://app.netlify.com/sites/prefect-orion/deploys/63b65bba566705000911e165
😎 Deploy Preview https://deploy-preview-7263--prefect-orion.netlify.app/api-ref/prefect/engine
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@zanieb zanieb added the enhancement An improvement of an existing feature label Nov 4, 2022
@zanieb zanieb marked this pull request as ready for review January 3, 2023 18:35
@@ -1418,7 +1412,7 @@ def resolve_input(expr):
# Do not allow uncompleted upstreams except failures when `allow_failure` has
# been used
if not state.is_completed() and not (
should_allow_failure and state.is_failed()
isinstance(context.get("annotation"), allow_failure) and state.is_failed()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. This will break when annotations are nested e.g. allow_failure(quote("foo")) — maybe I should make the context have a list or set of annotations?

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense to me - it seems that you could even change the key name to annotations without any backwards compatible breakage since the flow is re-parsed with each run

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'm going to wait for this to be an actual issue so we can keep this simple and get it out there

# Moved to `prefect.utilities.annotations` but preserved here for compatibility
from prefect.utilities.annotations import Quote, quote # noqa
# Quote moved to `prefect.utilities.annotations` but preserved here for compatibility
from prefect.utilities.annotations import BaseAnnotation, Quote, quote # noqa

Check notice

Code scanning / CodeQL

Unused import

Import of 'Quote' is not used. Import of 'quote' is not used.
@zanieb zanieb requested a review from peytonrunyan January 5, 2023 05:10
@zanieb
Copy link
Contributor Author

zanieb commented Jan 5, 2023

@peytonrunyan please re-review

Copy link
Contributor

@peytonrunyan peytonrunyan left a comment

Choose a reason for hiding this comment

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

lgtm!

@zanieb zanieb merged commit 7f5f707 into main Jan 6, 2023
@zanieb zanieb deleted the annotation-visit-2 branch January 6, 2023 17:05
github-actions bot pushed a commit that referenced this pull request Jan 6, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jan 6, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
masonmenges pushed a commit that referenced this pull request Jan 10, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jan 19, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jan 19, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jan 26, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jan 26, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
@zanieb zanieb mentioned this pull request Feb 3, 2023
3 tasks
zanieb added a commit that referenced this pull request Feb 3, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
zanieb added a commit that referenced this pull request Feb 3, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
zanieb added a commit that referenced this pull request Feb 6, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
zanieb added a commit that referenced this pull request Feb 6, 2023
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants