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

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

Merged
merged 38 commits into from Dec 28, 2022

Conversation

serinamarie
Copy link
Contributor

@serinamarie serinamarie commented Dec 16, 2022

Continuation of #7772

Notes

dateparser.parse has a default PREFER_DATES_FROM setting of current_period. This means that when parsing a date, it will go backwards in time if that's closer than a future time, e.g. if it is currently 5pm, then dateparser.parse("1pm") will retrieve a date from earlier today (in the past) because this 1pm is closer than tomorrow at 1pm.

By changing the PREFER_DATES_FROM setting to future like in this PR, when parsing a date, it will always go forward in time regardless of closeness, e.g. if it is currently 5pm, then dateparser.parse("1pm") will retrieve the next future instance of 1pm, meaning tomorrow at 1pm even though today at 1pm is closer in proximity.

We reasoned that most of the time, users want to run deployments at a future date, so we should consistently parse for a future date, rather than inconsistently provide a past date or future date based on proximity.

If needed someday, we could add a setting to enable other behavior.

Example

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

@serinamarie serinamarie changed the title Deployment start time continued Cleanup deployment-start-time feature branch, add tests Dec 16, 2022
@serinamarie serinamarie added the enhancement An improvement of an existing feature label Dec 19, 2022
@serinamarie serinamarie marked this pull request as ready for review December 19, 2022 17:20
@serinamarie serinamarie marked this pull request as draft December 19, 2022 17:39
requirements.txt Outdated
@@ -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.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the higher version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests were failing: https://github.com/PrefectHQ/prefect/actions/runs/3752658214/jobs/6375066682#step:12:236
I determined the cause to be: scrapinghub/dateparser#1045 and the solution was implemented here: scrapinghub/dateparser#1046.

Comment on lines +420 to +427
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
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)

Copy link
Contributor

@peytonrunyan peytonrunyan left a comment

Choose a reason for hiding this comment

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

A couple minor ux nits (not strong feelings) but otherwise lgtm!

@serinamarie serinamarie marked this pull request as ready for review December 27, 2022 21:15
@serinamarie serinamarie merged commit d9d8004 into deployment-start-time Dec 28, 2022
@serinamarie serinamarie deleted the deployment-start-time-continued branch December 28, 2022 18:09
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants