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.

Sign up for GitHub

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

Jump to bottom

Remove deployments module #13373

Merged
merged 51 commits into from
May 16, 2024
Merged

Remove deployments module #13373

merged 51 commits into from
May 16, 2024

Conversation

serinamarie
Copy link
Contributor

@serinamarie serinamarie commented May 15, 2024 •
edited

Closes #13358

  • Removes class Deployment and load_deployments_from_yaml
  • Moves run_deployment to flow_runs module
  • Move load_flow_from_flow_run to flows module
  • Moves tests from test_deployments to test_flow_runs and test_flows

@serinamarie serinamarie added the breaking This change is not backwards compatible label May 15, 2024
@serinamarie serinamarie linked an issue May 15, 2024 that may be closed by this pull request
Remove prefect.deployments.deployments.Deployment and load_deployments_from_yaml from the main branch #13358
Closed
Base automatically changed from remove-deployment-apply-cli to main May 15, 2024 17:25
serinamarie
serinamarie commented May 15, 2024
View reviewed changes
src/prefect/engine.py
@@ -147,7 +146,7 @@
PausedRun,
UpstreamTaskError,
)
from prefect.flows import Flow, load_flow_from_entrypoint
from prefect.flows import Flow, load_flow_from_entrypoint, load_flow_from_flow_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.

flows seemed a logical place to me for load_flow_from_flow_run to move (it's currently in deployments). Directly moved, no changes

serinamarie
serinamarie commented May 15, 2024
View reviewed changes
src/prefect/flow_runs.py Outdated
Comment on lines 116 to 117
@inject_client
async def run_deployment(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving run_deployment to flow_runs module made sense to me because it creates and returns a flow run. Directly moved, no changes.

Copy link
Contributor

@zzstoatzz zzstoatzz May 16, 2024 •
edited

Choose a reason for hiding this comment

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

hmm structurally doing from prefect.deployments import run_deployment makes a lot of sense to me, since flow runs dont need to have anything to do with deployments

do you think it might make sense to have something like prefect.deployments.flow_runs so we can easily expose run_deployment in prefect.deployments's __init__?

Copy link
Contributor

Choose a reason for hiding this comment

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

it also feels like if a prefect.deployments.flow_runs existed load_flow_from_flow_run might make sense there too

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 like that a lot! Good idea

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 moved run_deployment to deployments.flow_runs.py in 95a88a8, but since load_flow_from_flow_run is only used by the new_flow_engine, the runner, and the engine, I think it makes sense to keep moved to flows with the other load_flow_from_x, as we already import from there into the engine(s) and runner.

serinamarie
serinamarie commented May 15, 2024
View reviewed changes
tests/test_flow_runs.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moves tests from test_deployments to test_flow_runs.py. Superficial changes only.

serinamarie
serinamarie commented May 15, 2024
View reviewed changes
tests/test_flows.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moves tests from test_deployments to test_flows.py. Superficial changes only.

@serinamarie serinamarie changed the base branch from update-compat-tests to serina/update-compat-tests May 15, 2024 21:48
@serinamarie serinamarie marked this pull request as ready for review May 15, 2024 21:58
@serinamarie serinamarie requested review from desertaxle, zzstoatzz and a team as code owners May 15, 2024 21:58
desertaxle
desertaxle approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

👋

Base automatically changed from serina/update-compat-tests to main May 16, 2024 16:10
@serinamarie serinamarie merged commit cfff1d5 into main May 16, 2024
49 checks passed
@serinamarie serinamarie deleted the remove-deployments-module branch May 16, 2024 16:56
serinamarie added a commit to bsignoret/prefect that referenced this pull request May 16, 2024
@serinamarie
Remove deployments module (PrefectHQ#13373)
b0f9fbe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@desertaxle desertaxle desertaxle approved these changes

@zzstoatzz zzstoatzz zzstoatzz approved these changes

Assignees
No one assigned
Labels
breaking This change is not backwards compatible
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove prefect.deployments.deployments.Deployment and load_deployments_from_yaml from the main branch
3 participants
@serinamarie @desertaxle @zzstoatzz