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

Chore: add deprecation warning for artifacts created outside of runs #13688

Merged
merged 11 commits into from
Jul 22, 2024

Conversation

dylanbhughes
Copy link
Contributor

@dylanbhughes dylanbhughes commented May 30, 2024

Example

from prefect.artifacts import (
    create_image_artifact,
)


def create_image():
    # Do something to create an image and upload to a url
    image_url = "https://media3.giphy.com/media/v1.Y2lkPTc5MGI3NjExZmQydzBjOHQ2M3BhdWJ4M3V1MGtoZGxuNmloeGh6b2dvaHhpaHg0eSZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/3KC2jD2QcBOSc/giphy.gif"
    artifact_id = create_image_artifact(
        image_url=image_url, description="A gif.", key="image-outside-flow"
    )
    return image_url, artifact_id


if __name__ == "__main__":
    image_url, artifact_id = create_image()
    print(f"Image URL: {image_url}, artifact_id: {artifact_id}")
(prefect_playground) ╭─dylan@dylans-MacBook-Pro ~/dev/prefect_playground ‹main›
╰─$ python artifact_outside_flow_run.py
/Users/dylan/dev/prefect/src/prefect/artifacts.py:66: FutureWarning: Artifact creation outside of a flow or task run is deprecated and will be removed in a later version.
  warnings.warn(

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.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • This pull request includes a label categorizing the change e.g. maintenance, fix, feature, enhancement, docs.

For documentation changes:

  • This pull request includes redirect settings in mint.json for files that are removed or renamed.

For new functions or classes in the Python SDK:

  • This pull request includes helpful docstrings.
  • If a new Python file was added, this pull request contains a stub page in the Python SDK docs and an entry in docs/mint.json navigation.

@dylanbhughes dylanbhughes changed the title add deprecation warning for artifacts created without flow_run_id Chore: add deprecation warning for artifacts created outside of runs May 30, 2024
@dylanbhughes dylanbhughes added the development Tech debt, refactors, CI, tests, and other related work. label May 30, 2024
@dylanbhughes dylanbhughes marked this pull request as ready for review May 31, 2024 15:23
@dylanbhughes dylanbhughes requested a review from a team as a code owner May 31, 2024 15:23
Copy link
Contributor

This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment.

Copy link
Contributor

@serinamarie serinamarie left a 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

@serinamarie
Copy link
Contributor

Although there seem to be 14 failing tests for this error:

> >       raise MissingContextError(
            "No run context available. You are not in a flow or task run context."
        )
E       prefect.exceptions.MissingContextError: No run context available. You are not in a flow or task run context.

src/prefect/context.py:545: MissingContextError

@zzstoatzz
Copy link
Collaborator

chatted sync already but lgtm besides the filter (i think we should update the tests instead)

Copy link

codspeed-hq bot commented Jul 12, 2024

CodSpeed Performance Report

Merging #13688 will not alter performance

Comparing chore/artifact-outside-flowrun-deprecation-warning (83c0f23) with main (9a3dc73)

Summary

✅ 5 untouched benchmarks

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

maybe we could add a test for the deprecation warning case? and just out of curiosity, why FutureWarning instead of DeprecationWarning?

@dylanbhughes
Copy link
Contributor Author

maybe we could add a test for the deprecation warning case? and just out of curiosity, why FutureWarning instead of DeprecationWarning?

@zzstoatzz I will add a test for the warning 👍🏻

I used FutureWarning based on the description (seemed to apply here) and because when I called a flow from the terminal with python my_flow.py I didn't see the warning log for DeprecationWarning but I did see it for FutureWarning. Is there a different setting I should be using?

@zzstoatzz
Copy link
Collaborator

maybe we could add a test for the deprecation warning case? and just out of curiosity, why FutureWarning instead of DeprecationWarning?

@zzstoatzz I will add a test for the warning 👍🏻

I used FutureWarning based on the description (seemed to apply here) and because when I called a flow from the terminal with python my_flow.py I didn't see the warning log for DeprecationWarning but I did see it for FutureWarning. Is there a different setting I should be using?

on FutureWarning, makes sense to me!

@dylanbhughes dylanbhughes merged commit e913e75 into main Jul 22, 2024
33 checks passed
@dylanbhughes dylanbhughes deleted the chore/artifact-outside-flowrun-deprecation-warning branch July 22, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Tech debt, refactors, CI, tests, and other related work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants