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

Flow with_options fixes - retries, retry_delay_seconds, flow_run_name #12020

Merged
merged 7 commits into from
Feb 22, 2024

Conversation

NodeJSmith
Copy link
Contributor

@NodeJSmith NodeJSmith commented Feb 18, 2024

Closes #11468

Correct logic in Flow.with_options to allow setting retries and retry_delay_seconds to 0.

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. maintenance, fix, feature, enhancement, docs.

For documentation changes:

  • This pull request includes redirect settings in netlify.toml 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 mkdocs.yml navigation.

@NodeJSmith NodeJSmith requested a review from a team as a code owner February 18, 2024 02:31
Copy link

netlify bot commented Feb 18, 2024

👷 Deploy request for prefect-docs-preview pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 518e792

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.

Thanks for the PR @NodeJSmith! Could you please add one or two regression tests covering these changes?

Copy link
Contributor

@bunchesofdonald bunchesofdonald left a comment

Choose a reason for hiding this comment

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

@NodeJSmith thank you for the pull request! The code here looks right to me, and I love that you fixed the type hint as well. I have one ask, could you add these options to this existing test? https://github.com/PrefectHQ/prefect/blob/main/tests/test_flows.py#L272

@bunchesofdonald bunchesofdonald added the status:waiting for user This needs a response or changes from the user or contributor label Feb 20, 2024
@NodeJSmith
Copy link
Contributor Author

@bunchesofdonald Thanks for pointing out where to add these, I had meant to do that before creating the PR, I think it slipped my mind. Two tests added, one for each of the attributes. I double checked on the logic, since the timeout_seconds override test asserts that timeout_seconds gets set to None, not 0. I think the logic is sound though, the retries/retry_delay_seconds are just a bit different from the timeout_seconds as far as how they are set in the __init__ method.

Let me know if you have any other concerns or if I need to add anything else.

@desertaxle desertaxle added the fix A fix for a bug in an existing feature label Feb 21, 2024
@NodeJSmith
Copy link
Contributor Author

@bunchesofdonald I just realized while working on something that flow_run_name doesn't follow the convention of taking the value from the original Flow if not provided to with_options. Is this by design? If not I'm happy to add that to this PR

@bunchesofdonald
Copy link
Contributor

bunchesofdonald commented Feb 21, 2024

@bunchesofdonald I just realized while working on something that flow_run_name doesn't follow the convention of taking the value from the original Flow if not provided to with_options. Is this by design? If not I'm happy to add that to this PR

I don't believe that was by design, so if you want to make this PR include that as well that seems fine to me.

Also don't worry about these test failures, httpx released a new version and we're dealing with some deprecation warnings.

@NodeJSmith NodeJSmith changed the title allow retries and retry_delay_seconds to be set using with_options Flow with_options fixes - retries, retry_delay_seconds, flow_run_name Feb 22, 2024
@NodeJSmith
Copy link
Contributor Author

@bunchesofdonald Ready for review again, fixed the flow_run_name argument and added a test validating the behavior

@bunchesofdonald bunchesofdonald added status:waiting for maintainer This needs a response from a Prefect maintainer and removed status:waiting for user This needs a response or changes from the user or contributor labels Feb 22, 2024
Copy link
Contributor

@bunchesofdonald bunchesofdonald left a comment

Choose a reason for hiding this comment

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

This is great, thank you @NodeJSmith!

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.

LGTM!

@bunchesofdonald bunchesofdonald merged commit 4fda813 into PrefectHQ:main Feb 22, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A fix for a bug in an existing feature 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.

Cannot set retries to 0 using with_options for flow
3 participants