-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Feature: JSON Artifacts #12295
Feature: JSON Artifacts #12295
Conversation
✅ Deploy Preview for prefect-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Makes sense to me! I like using a class method for creation. It makes it feel more like it's making a thing
@jakekaplan want to give it another once over? |
Arguments: | ||
type: A string identifying the type of artifact. | ||
key: A user-provided string identifier. | ||
The key must only contain lowercase letters, numbers, and dashes. | ||
description: A user-specified description of the artifact. | ||
data: A JSON payload that allows for a result to be retrieved. | ||
""" |
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.
should we note the optional args here?
data: Optional[Union[Dict[str, Any], Any]] = None, | ||
client: Optional[PrefectClient] = None, | ||
**kwargs: Any, | ||
) -> Tuple[ArtifactResponse, bool]: |
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.
What would you think about this just returning the artifact? It feels more unified with the other methods to keep the response shape/type the same.
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.
I am part of the anti-tuple response movement 🙃
await client.read_artifacts( | ||
limit=1, | ||
sort=ArtifactSort.UPDATED_DESC, | ||
artifact_filter=ArtifactFilter(key=ArtifactFilterKey(any_=[key])), |
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.
I think you'll need to add a client method for it but there is a more efficient lookup here:
https://github.com/PrefectHQ/prefect/blob/main/src/prefect/server/api/artifacts.py#L64-L80
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.
Couple comments, otherwise LGTM
Co-authored-by: Andrew Brookins <andrew.b@prefect.io>
Mostly a refactor of our public artifact methods.
The biggest change is a new public class-based interface to Artifacts, which:
json
.get
by key, which was previously available only through the CLI.get_or_create
by key, which does what you think it does.I'm doing a draft PR