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

[enhancement]: Add pull_steps to the base Deployment model #9233

Closed

Conversation

davzucky
Copy link

@davzucky davzucky commented Apr 17, 2023

Following an issue related to remote storage in Kubernetes @cicdw pointed me to the new concept of project. Looking at the code project introduces a new concept to the deployment model, which is pull_steps. This exists on the ORM model but is not exposed to the prefect.Deployment model. This change a the pull_steps to the base mdel.
Another issue about that topic is open #9220

Example

This will allow to create deployment using build_from_flow

from my_project.flows import my_flow
from prefect.deployments import Deployment

deployment = Deployment.build_from_flow(
      flow=my_flow,
      name="example",
      version="1",
      tags=["demo"],
      pull_steps=[{"prefect.project.steps.pull.set_working_directory": {"directory": "/prefect/workdir"}}]
      )

deployment.apply()

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, docs.

@davzucky davzucky requested a review from a team as a code owner April 17, 2023 01:01
@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for prefect-docs-preview ready!

Name Link
🔨 Latest commit 3de2326
🔍 Latest deploy log https://app.netlify.com/sites/prefect-docs-preview/deploys/643c9fd5e35361000845d4f0
😎 Deploy Preview https://deploy-preview-9233--prefect-docs-preview.netlify.app/api-ref/prefect/deployments
📱 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.

@cicdw
Copy link
Member

cicdw commented Apr 17, 2023

My only hesitation here is that pull_steps are not fully compatible with blocks and the logic in build_from_flow will currently allow for setting all 3 fields simultaneously which could lead to unexpected outcomes - could you introduce some early error logic within that method to avoid this confusion?

@davzucky
Copy link
Author

Indeed that makes sense that build_from_flow can be confused. However, as the project still relies on the deployment object, I can see three options:

  1. add pre-check to ensure build_from_flow that is (infrastructure, storage) are not set when (worker_pool, pull_steps) are set
  2. Create a new function builld_from_project that would limit the field expose and make it clear to be built for the new way to deploy
  3. Add a validator for pull_steps and worker_pool

Am I missing something related to the change you are making with Project that I need to be aware of?
What would be your preference on the implementation side?

@zanieb zanieb added the enhancement An improvement of an existing feature label Apr 18, 2023
@cicdw
Copy link
Member

cicdw commented Apr 19, 2023

To be totally honest, we wanted to entirely avoid a Python SDK for projects so I'm not sure the best way to proceed here - in the same way that you run testing suites from a CLI, not from within one of the test files, it really complicates things to expose ways of deploying projects from within a file (or from within a REPL, which people often try but it fails because there's no natural filesystem to base things on).

@davzucky
Copy link
Author

I can see where you come from, and keeping the API level small help a lot
Our process today is using prefect flow to set up prefect flow and blocks (like 'Inception") This makes our deployment testable, versioned, and reproducible since everything is embedded in our code.

Today, with Prefect, we had the choice between using Prefect command line,Rest API, or Python Api. If you create a new paradigm, it is essential that we can still have access at all these levels

Looking more at Project, It needs more capabilities today to be helpful.

  • We want to deploy once, and this deploys all the flows we need
  • Block and deployment need to be able to be deployed together
  • Multiple docker registries (Dev to push), and we can only promote to prod one.
  • We need to be able to set up which deployment we enable in different environments

Happy to have a chat about how we are doing that.

@chrisguidry chrisguidry added needs:triage Needs feedback from the Prefect product team status:waiting for maintainer This needs a response from a Prefect maintainer labels Jun 12, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the status:stale This may not be relevant anymore label Aug 11, 2023
@cicdw
Copy link
Member

cicdw commented Aug 15, 2023

Returning to this -- I still think #9233 (comment) articulates why we can't merge the PR as-is, but we are actively experimenting with streamlined deployment UX (including Python-first deployments) that will take this feedback into account!

@github-actions github-actions bot removed the status:stale This may not be relevant anymore label Aug 15, 2023
@github-actions
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.

@github-actions github-actions bot added the status:stale This may not be relevant anymore label Aug 29, 2023
@davzucky
Copy link
Author

Thank you, @cicdw for the update. Do you have an issue we can follow regarding your current work on that side?

@github-actions github-actions bot removed the status:stale This may not be relevant anymore label Aug 30, 2023
@cicdw
Copy link
Member

cicdw commented Aug 30, 2023

Absolutely @davzucky - this PR is the first major milestone in that Python-first direction: #10534

It probably isn't obvious from the PR that it's related, but we are filling in the incremental gaps that lead up to a full worker / work pool setup and hardening the Python SDK for deployments as we go:

  • the linked PR introduces a mechanism that allows for Python-based workflows to be deployed without workers or work pools (this is intended to be the easiest way to get a deployment setup for situations that don't need dynamically provisioned infrastructure, so it's all pure Python)
  • The next milestone will be to expose a way to handle workflows defined in remote filesystems, but still in the "no dynamic infra" world (i.e., no workers and work pools). This is when Python configuration for pull_steps will show up
  • the last step will be to provide mechanisms for transitioning deployments defined in this highly Python-centric way to the dynamic and decoupled worker / work pool world (the current world)

We are targeting end of September for completion of the core components of this work (so roughly one month from now)

@github-actions
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.

@github-actions github-actions bot added the status:stale This may not be relevant anymore label Sep 13, 2023
@desertaxle desertaxle removed the needs:triage Needs feedback from the Prefect product team label Sep 20, 2023
@github-actions github-actions bot removed the status:stale This may not be relevant anymore label Sep 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

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.

@github-actions github-actions bot added the status:stale This may not be relevant anymore label Oct 4, 2023
@github-actions
Copy link
Contributor

This pull request was closed because it has been stale for 14 days with no activity. If this pull request is important or you have more to add feel free to re-open it.

@github-actions github-actions bot closed this Oct 18, 2023
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 status:stale This may not be relevant anymore status:waiting for maintainer This needs a response from a Prefect maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants