Relative initial cycle point

[lhuggett]

Closes #984.

NOTE: includes latest version of isodatetime, and changes to Cylc from @kinow for compatibility with latest version of isodatetime

NOTE 2: relative ICP may break when order of tick over in isodatetime is swapped as in metomi/isodatetime#101.

NOTE 3: fully updated documentation with information on use of days, weeks, months and years in relative ICP to follow.

[trwhitcomb]

This is great, and solves some problems that I've had crop up recently. One thing I still have questions on is whether it solves the problem of wanting the suite to start at the next integer multiple of some frequency (i.e. if I have tasks cycling every PT15M, I'd want to start the suite at 0, 15, 30, or 45 past the hour).

It looks like from the documentation added that such a thing is possible, but requires the times to be explicitly listed (T-00,T-15,T-30, etc.). Is that correct?

[lhuggett]

This is great, and solves some problems that I've had crop up recently. One thing I still have questions on is whether it solves the problem of wanting the suite to start at the next integer multiple of some frequency (i.e. if I have tasks cycling every PT15M, I'd want to start the suite at 0, 15, 30, or 45 past the hour).

It looks like from the documentation added that such a thing is possible, but requires the times to be explicitly listed (T-00,T-15,T-30, etc.). Is that correct?

Yes, that's right. That particular one's in the documentation, so you could just copy and paste it into your suite.rc to save typing it yourself. It might get a bit tedious typing it out if you want it to start on the next 5 minute interval, though...

[hjoliver]

@lhuggett - is this ready to review? (also, note conflicts)

[lhuggett]

@lhuggett - is this ready to review? (also, note conflicts)

It was when I pushed it, but I guess it needs more work now to deal with conflicts.

[kinow]

@lhuggett you can rebase your branch to edit commits, and drop the ones with my change... then fetch all, and rebase on top of master. That should make your branch have the latest version of isodatetime, plus the fixes for compatibility 👍

Sorry again for the trouble.

Bruno

[matthewrmshin]

@kinow @oliver-sanders please review. It would be nice if this can be included in the next release.

[oliver-sanders]

Some minor syntax comments to get the ball rolling, now I need to get my head around datetimes...

[matthewrmshin]

(We may have to push this back to soon.)

[kinow]

@matthewrmshin, I'm waiting for @lhuggett to take a look at @oliver-sanders comments, to take a look at the changes here after that. But agree it might be easier to move to soon.

[hjoliver]

All - I've pushed this back to "soon" because things are getting desperate on the release front. Feel free to promote back to "next release" if you can complete reviews in time...

[matthewrmshin]

No problem.

[matthewrmshin]

@oliver-sanders @kinow this branch should now be ready for review again.

[kinow]

Really great work! 👏 👏 👏

Built the manual with make to have a look at the final PDF, and found one typo only. Very well written, and easy to understand. Thanks for putting the time to write good documentation.

The tests are also a great documentation (at least for me). Bonus points for adding a functional test testing the system from point to point with the new feature.

The unit tests look great. Found one issue with the initialization of some constants. Running the test classes individually does not work (see comments).

The coverage is good, but there are a few branches in the new ingest_time function that were never tested. Added some examples that should bring the line/branch coverage up to 100% in the ingest_time (does not mean it is bug free though 🐛 ).

Only other minor nit-pick I have is still about tests. From what I could tell, this PR changes the SuiteConfig in config.py, and CylcConfigValidator in cfgvalidate.py. However, it looks to me like the new code is not being tested (especially the cases that may cause exceptions).

It looks to me like this is doable, though may require mocking some objects for a unit test, or crafting a functional test. If you or others think that testing those methods is not important, and should not be a blocker, then all good for me (I don't know much about those files right now, and we don't need to cover every corner of the code).

Executed the following suite:

testuser@cylc:~/suites$ cat pbs2/suite.rc 
#[cylc]
#    UTC mode = true

[scheduling]
    initial cycle point = next(T-51)
    [[dependencies]]
        [[[P1D]]]
            graph = t1
[runtime]
    [[t1]]
        script = "echo \"The time is $(date +'%T')\""
        [[[remote]]]
            host=pbs
        [[[job]]]
            batch system = pbs

On a PBS cluster:

testuser@cylc:~/suites$ cylc cat-state pbs2
run mode : live
time : 2019-01-04T02:49:27Z (1546570167)
initial cycle : 20190104T0251Z
final cycle : None
(dp0
.
Begin task states
t1.20190105T0251Z : status=succeeded, spawned=True
t1.20190106T0251Z : status=ready, spawned=False


testuser@cylc:~/suites$ cylc cat-log pbs2 t1.20190104T0251Z
Suite    : pbs2
Task Job : 20190104T0251Z/t1/01 (try 1)
User@Host: testuser@pbs

"The time is 02:49:23"
2019-01-04T02:49:23Z INFO - started
2019-01-04T02:49:24Z INFO - succeeded

It executed on the next T-51 (i.e. next time minutes on the click ticked past 51, though my system is a mix of NZ time and UTC, causing a mess, but it worked as expected [I think]). Then the next cycle point was for tomorrow at the same time. So I believe everything is working as expected, even with remote tasks. Well done! After the typo is fixed, and if others or @lhuggett could confirm the amount of tests is good enough, I'll change the review to Approve.

Cheers
Bruno

[oliver-sanders]

Code-wise this looks good to me. The tests are pretty extensive, good to see so many edge cases covered.

[edit] Irrelevant

There is a bit of a gap in the coverage in the cfgvalidate file (in the block which starts for i in range(1, 101)) which would be good to cover.

[oliver-sanders]

🍾 I think we are there! Looks good, tests pass!

A note for the future, it would be good to hook this functionality into cylc cyclepoint or cylc date or whatever so people can play with this syntax on the command line.

[matthewrmshin]

🎉 🎉 🎉