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

Cleanup deployment-start-time feature branch, add tests #7928

Merged
merged 38 commits into from Dec 28, 2022
Merged

Cleanup deployment-start-time feature branch, add tests #7928

Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
87eb843
Update docstring
serinamarie Dec 15, 2022
b108a7d
Cleanup start-in start-at param
serinamarie Dec 16, 2022
97d41d6
Move start_in, start_at below other params
serinamarie Dec 16, 2022
7ab9503
Move human_readable_dt_diff assigning
serinamarie Dec 16, 2022
637ad4a
Add test template
serinamarie Dec 16, 2022
bd0f1c3
Rename test
serinamarie Dec 16, 2022
ab00c3e
Rename tests
serinamarie Dec 16, 2022
2016455
Start implementing tests for scheduling behavior
zanieb Dec 16, 2022
3a709d3
Add test for invalid input
zanieb Dec 16, 2022
e5c73f0
Add more tests
serinamarie Dec 19, 2022
58c2217
Fix tests
serinamarie Dec 19, 2022
1c1f555
Rename long var name
serinamarie Dec 19, 2022
4ff7d46
Handle unexpected exceptions
serinamarie Dec 19, 2022
259bec9
Add more tests
serinamarie Dec 19, 2022
3eeeb55
Add exception handling when user input includes tz
serinamarie Dec 19, 2022
9370e2a
Update test_start_at_option_schedules_flow_run_in_future
serinamarie Dec 19, 2022
23d36dd
Modify tests
serinamarie Dec 20, 2022
be6ff76
Accommodate timezones
serinamarie Dec 20, 2022
5aa10f3
Some cleanup
serinamarie Dec 20, 2022
180fd45
Remove diff_for_humans because of 1 second lag
serinamarie Dec 20, 2022
8a77b92
Remove 2nd parsing for display, add future pref in dateparsing
serinamarie Dec 21, 2022
6b4f94d
add test, change expected values due to future dateparser setting
serinamarie Dec 21, 2022
1f9f23e
Fix tests, hopefully
serinamarie Dec 21, 2022
927deba
Remove 2nd calculation of local tz
serinamarie Dec 21, 2022
74503df
Cleanup tests
serinamarie Dec 22, 2022
55bfce4
Cleanup
serinamarie Dec 22, 2022
d089ddb
bump dateparser version
serinamarie Dec 22, 2022
3ef506a
Fix tests
serinamarie Dec 22, 2022
104e72b
rename tests, test var
serinamarie Dec 22, 2022
8912f52
Modify print statements, docstrings
serinamarie Dec 27, 2022
4b80352
Put parsing logic for start_time params in a helper function
serinamarie Dec 27, 2022
3320144
Rename helper function
serinamarie Dec 28, 2022
07c7cb7
Update src/prefect/cli/deployment.py
serinamarie Dec 28, 2022
b205827
Update src/prefect/cli/deployment.py
serinamarie Dec 28, 2022
02b9f14
Remove helper function
serinamarie Dec 28, 2022
358adb7
Add test to document start-in behavior
serinamarie Dec 28, 2022
eaa14c5
Correct test
serinamarie Dec 28, 2022
44f03b4
Modify exit error & corresponding test
serinamarie Dec 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ cloudpickle >= 2.0
coolname >= 1.0.4
croniter >= 1.0.12
cryptography >= 36.0.1
dateparser >= 1.0
dateparser >= 1.1.1
docker >= 4.0
fastapi >= 0.70
fsspec >= 2022.5.0
Expand Down
74 changes: 41 additions & 33 deletions src/prefect/cli/deployment.py
View file
Open in desktop
Expand Up @@ -384,7 +384,8 @@ async def run(
"""
Create a flow run for the given flow and deployment.

The flow run will be scheduled for now and an agent must execute it.
The flow run will be scheduled for now if start-in or start-at are unspecified;
and an agent must execute it.
serinamarie marked this conversation as resolved.
Show resolved Hide resolved

The flow run will not execute until an agent starts.
"""
Expand All @@ -411,31 +412,46 @@ async def run(
)
parameters = {**multi_params, **cli_params}

# Parse the start time if provided
if start_in:
start_time_raw = "in " + start_in
elif start_at:
start_time_raw = "at " + start_at
else:
start_time_raw = None
if start_in and start_at:
exit_with_error(
"Expected optional start_in field or start_at field but not both."
serinamarie marked this conversation as resolved.
Show resolved Hide resolved
)

if start_time_raw:
elif start_in is None and start_at is None:
scheduled_start_time = now
human_dt_diff = " (now)"
else:
if start_in:
start_time_raw = "in " + start_in
else:
start_time_raw = "at " + start_at
Comment on lines +418 to +425
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the above exits if both are set you can just do

if start_in: ... 
elif start_at: ...
else: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean else: being where start_in is None and start_as in None? Because there is some logic beneath that specific to start_in/start_at for assigning start_time_parsed, scheduled_start_time, and human_dt_diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think you can do else instead of elif start_in is None and start_at is None:

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious too what this would look like without having a helper function for the raw parsing logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the helper function is a bit harder to read than the initial implementation, especially since it does more than one thing (parses the date time and generates a formatted diff). I don't have strong feelings here though.

Copy link
Contributor

Choose a reason for hiding this comment

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

(unresolving for visibility; does not mean it needs to be addressed)

with warnings.catch_warnings():
# PyTZ throws a warning based on dateparser usage of the library
# See https://github.com/scrapinghub/dateparser/issues/1089
warnings.filterwarnings("ignore", module="dateparser")
start_time_parsed = dateparser.parse(
start_time_raw,
settings={
"TO_TIMEZONE": "UTC",
"RELATIVE_BASE": datetime.fromtimestamp(now.timestamp()),
},
)

try:

start_time_parsed = dateparser.parse(
start_time_raw,
settings={
"TO_TIMEZONE": "UTC",
"RETURN_AS_TIMEZONE_AWARE": False,
"PREFER_DATES_FROM": "future",
"RELATIVE_BASE": datetime.fromtimestamp(now.timestamp()),
},
)

except Exception as exc:
exit_with_error(f"Failed to parse '{start_time_raw!r}': {exc!s}")

if start_time_parsed is None:
exit_with_error(f"Unable to parse scheduled start time {start_time_raw!r}.")

scheduled_start_time = pendulum.instance(start_time_parsed)
else:
scheduled_start_time = now
human_dt_diff = (
" (" + pendulum.format_diff(scheduled_start_time.diff(now)) + ")"
)

async with get_client() as client:
deployment = await get_deployment(client, name, deployment_id)
Expand Down Expand Up @@ -474,21 +490,13 @@ async def run(
else:
run_url = "<no dashboard available>"

if start_in:
scheduled_display = (
scheduled_start_time.in_tz(
pendulum.tz.local_timezone()
).to_datetime_string()
+ " ("
+ pendulum.format_diff(scheduled_start_time.diff(now))
+ ")"
)
elif start_at:
scheduled_display = scheduled_start_time.in_tz(
pendulum.tz.local_timezone()
).to_datetime_string()
else:
scheduled_display = "now"
datetime_local_tz = scheduled_start_time.in_tz(pendulum.tz.local_timezone())
scheduled_display = (
datetime_local_tz.to_datetime_string()
+ " "
+ datetime_local_tz.tzname()
+ human_dt_diff
)

app.console.print(f"Created flow run {flow_run.name!r}.")
app.console.print(
Expand Down