-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
add prefect.yaml
and cli support for new schedule fields
#13318
Conversation
✅ Deploy Preview for prefect-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…thub.com/prefecthq/prefect into new-schedule-fields-yaml-and-cli-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few minor nits, but looks great overall!
command=( | ||
f"deploy ./flows/hello.py:my_flow -n test-name --pool {work_pool.name}" | ||
), | ||
user_input=( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't block this PR on this, but a couple weeks ago I added a more semantic way of dealing with this in tests called prompts_and_responses
, you can see an example over here: https://github.com/PrefectHQ/prefect/blob/main/tests/cli/test_deploy.py#L4130C13-L4140
src/prefect/cli/_prompts.py
Outdated
default=None, | ||
) | ||
catchup = confirm( | ||
"Catch up on missed runs?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe include the fact that we're talking about 'late' runs here?
"Catch up on missed runs?", | |
"Catch up on late flow runs?", |
src/prefect/cli/_prompts.py
Outdated
@@ -238,6 +242,21 @@ def process_response(self, value: str) -> str: | |||
raise InvalidResponse(self.validate_error_message) | |||
|
|||
|
|||
def prompt_for_max_active_runs_and_catchup(console): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd split this into two prompts, one for max active
and one for catchup
.
src/prefect/cli/_prompts.py
Outdated
@@ -376,7 +405,7 @@ def prompt_schedules(console) -> List[MinimalDeploymentSchedule]: | |||
async def prompt_select_work_pool( | |||
console: Console, | |||
prompt: str = "Which work pool would you like to deploy this flow to?", | |||
client: PrefectClient = None, | |||
client: Optional[PrefectClient] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing to not block this PR on, but I added a client_injector
that makes the typing on this kind of stuff better:
prefect/src/prefect/client/utilities.py
Lines 66 to 74 in 8454a1c
def client_injector( | |
func: Callable[Concatenate["PrefectClient", P], Awaitable[R]], | |
) -> Callable[P, Awaitable[R]]: | |
@wraps(func) | |
async def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: | |
client, _ = get_or_create_client() | |
return await func(client, *args, **kwargs) | |
return wrapper |
It's used the same way as inject_client
except that it always expects the client
will be the first argument and non-optional:
prefect/src/prefect/input/actions.py
Lines 54 to 57 in 8454a1c
@sync_compatible | |
@client_injector | |
async def create_flow_run_input( | |
client: "PrefectClient", |
version: str = None, | ||
schedule: SCHEDULE_TYPES = None, | ||
schedules: List[DeploymentScheduleCreate] = None, | ||
version: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed I would split up the prompts, but I have no blocking comments. Looks good!
…thub.com/prefecthq/prefect into new-schedule-fields-yaml-and-cli-support
this PR:
PREFECT_EXPERIMENTAL_ENABLE_SCHEDULE_CONCURRENCY
to gate CLI prompts for new fields while the worker does not actually enforce these limitsprefect deploy
to choose a value formax_active_runs
andcatchup
for each scheduleprefect.yaml