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

#119: fix clock-triggering and restore local time usage #954

Merged
merged 12 commits into from
May 22, 2014

Conversation

benfitzpatrick
Copy link
Contributor

This fixes a few flaws in the way time zones are handled. It also
fixes clock triggering (the motivation).

At the moment, under the ISO 8601 cycling, all output and internal
cycle times are in UTC by default. Cylc used to use local time, and
this is really the 'normal' way to consider a non-zoned date/time.
It is also a more common representation in the ISO 8601 standard.

This change uses local time by default unless the UTC flag is set
or a particular time zone is set. This also affects input cycle
points that appear without time zones.

This implies that we need to set some time zone information in
most of our reference tests - otherwise we will disagree on the
results in the reference logs from location to location and from
DST to not-DST. I thought about making UTC a default for
reference tests, but it didn't seem quite right - and there are
some tests that shouldn't be in UTC, including a new local time
test and the clock triggering ones.

This change also updates isodatetime to a newer version with
better time zone assumptions (what a coincidence...).

[EDIT: also fixed sequential, fixed cylc-insert, shutdown/00-cycle,
restart/03-retrying, and added skipping for async-satellite and
one-off tests]

@hjoliver, please review.

This fixes a few flaws in the way time zones are handled. It also
fixes clock triggering (the motivation).

At the moment, under the ISO 8601 cycling, all output and internal
cycle times are in UTC by default. Cylc used to use local time, and
this is really the 'normal' way to consider a non-zoned date/time.

This change uses local time by default unless the UTC flag is set
or a particular time zone is set.

This implies that we need to set some time zone information in
most of our reference tests - otherwise we will disagree on the
results in the reference logs from location to location and from
DST to not-DST. I thought about making UTC a default for
reference tests, but it didn't seem quite right - and there are
some tests that shouldn't be in UTC, including a new local time
test and the clock triggering ones.

This change also updates isodatetime to a newer version with
better time zone assumptions (what a coincidence...).
@benfitzpatrick benfitzpatrick added this to the cylc-5.4.14 milestone May 7, 2014
@benfitzpatrick benfitzpatrick modified the milestones: cylc-6, cylc-5.4.14 May 7, 2014
@benfitzpatrick
Copy link
Contributor Author

(This needs to wait for metomi/isodatetime#43, really.)

@hjoliver
Copy link
Member

@benfitzpatrick - I've had a look through this, can't see any problems.

But:

(This needs to wait for metomi/isodatetime#43, really.)

I'm taking this literally until advised otherwise!

@benfitzpatrick
Copy link
Contributor Author

Updated, but needs another test-battery run through for peace of mind...

@benfitzpatrick
Copy link
Contributor Author

OK, only the following tests fail for me now:

intercycle/01-future.t
purge/00-purge.t
restart/03-retrying.t

I've added auto-skipping above for some tests that I think don't matter.

OK to review.

@benfitzpatrick
Copy link
Contributor Author

intercycle/01-future.t is broken until implicit cycling comes back.
purge/00-purge.t is broken because (I think) the 18 cycle is not purged
as it used to be, probably due to the runahead pool change - I think
this is more of a change in behaviour rather than a bug, but not sure yet.
restart/03-retrying.t is broken because the try number seems to over-increment
in the database.

@benfitzpatrick
Copy link
Contributor Author

Actually, looking at master, the retrying test appears to be doing the
right thing by over-incrementing....

@benfitzpatrick
Copy link
Contributor Author

I'll assign this to @matthewrmshin for final review.

@@ -322,6 +322,20 @@ def get_prev_point( self, p ):
p_prev = p - self.i_step
return self._get_point_in_bounds( p_prev )

def get_nearest_prev_point(self, p):
Copy link
Contributor

Choose a reason for hiding this comment

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

p not a good name.

@matthewrmshin
Copy link
Contributor

Code change looks OK, apart from some minor style issues. I have 2 unexpected test failures, however:

  • restart/09-running-ll.t
  • restart/10-submit-failed-ll.t

@benfitzpatrick
Copy link
Contributor Author

Style issues addressed - please try re-running those tests with the updated branch.

@matthewrmshin
Copy link
Contributor

Now looking good. I now only have test failures in the 2 expected.

matthewrmshin added a commit that referenced this pull request May 22, 2014
#119: fix clock-triggering and restore local time usage
@matthewrmshin matthewrmshin merged commit 006f9ca into cylc:119.iso8601-cycling May 22, 2014
benfitzpatrick added a commit to benfitzpatrick/cylc that referenced this pull request May 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants