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

No constants #1730

Merged
merged 13 commits into from
Nov 15, 2019
Merged

No constants #1730

merged 13 commits into from
Nov 15, 2019

Conversation

cicdw
Copy link
Member

@cicdw cicdw commented Nov 12, 2019

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • adds new tests (if appropriate)
  • updates CHANGELOG.md (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Note that your PR will not be reviewed unless all three boxes are checked.

What does this PR change?

This PR completely avoids the auto-generation and execution of Python constants which are upstream of a given task. Instead, these constants are tracked on the Flow object directly, and artificial edges are created at runtime to manage them as upstream dependencies.

Why is this PR important?

The autogeneration of constants is perhaps one of the more confusing aspects of Prefect's functional API for newcomers. In addition, Cloud users do not find Constant[str] type tasks to be useful or informative, and ultimately these just eat up real estate on all screens. The current PR completely avoids this situation unless a user explicitly opts into it via wrapping their objects in prefect.tasks.core.constants.Constant()

Labeled as WIP as I still need to:

  • add more tests
  • update documentation
  • handle Cloud retries / failures

but I wanted some initial feedback on the implementation here.

@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #1730 into master will decrease coverage by <.01%.
The diff coverage is 95.83%.

@joshmeek
Copy link

What's the key noticeable difference that one would see when this is merged? Am I thinking about it right in that we won't see all the Constant task run logs anymore unless we declare Constants in our flow?

@cicdw
Copy link
Member Author

cicdw commented Nov 13, 2019

@joshmeek yup that's one major difference; another is that

with Flow("blah") as flow:
    add(2, 4)

will result in a Flow with a single task. This will be very apparent in flow.visualize() and in the UI views.

@cicdw cicdw changed the title [WIP] No constants No constants Nov 14, 2019
@cicdw cicdw removed the WIP label Nov 14, 2019
@cicdw
Copy link
Member Author

cicdw commented Nov 14, 2019

@jlowin / @joshmeek this is ready for review: note that I am choosing to not implement a serializer for ConstantResultHandler at this time because the planned cadence of our releases could cause issues for people who want to start working off 0.7.2 once it's released later this week.

joshmeek
joshmeek previously approved these changes Nov 14, 2019
Copy link

@joshmeek joshmeek left a comment

Choose a reason for hiding this comment

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

Looks good from my standpoint

jlowin
jlowin previously requested changes Nov 14, 2019
src/prefect/engine/flow_runner.py Outdated Show resolved Hide resolved
Co-Authored-By: Jeremiah Lowin <153965+jlowin@users.noreply.github.com>
@cicdw cicdw merged commit 42636b9 into master Nov 15, 2019
@cicdw cicdw deleted the no-constants branch November 15, 2019 17:31
zanieb pushed a commit that referenced this pull request May 12, 2022
Admin: Bumping orion-design version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants