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

#976: advanced dependencies #1044

Merged

Conversation

benfitzpatrick
Copy link
Contributor

This closes #976 by adding a complex dependency test and adding
the code to make it work.

The test contains statements like:

        [[[30T06+P8DT6H/P3W]]]
            graph = "foo[20T-P2W] => bar"

The parsing and processing of things like 20T-P2W was already done in the
time_parser.py for recurrence statements, but can now be done via a
convenient get_point_relative call.

The major issue is that with statements like foo[20T-P2W] => bar, it is no
longer possible to figure out a cleanup cutoff or a future trigger offset that
is universally valid for all points on the sequence. This now has to be done
for each task instance in taskdef.py. For standard, simple offsets like
[-PT6H], the behaviour is pretty much the same.

The new future trigger runahead treatment has the nice property of being
adaptive based on the current active tasks.

@hjoliver, please review.

@benfitzpatrick benfitzpatrick added this to the cylc-6 milestone Jul 30, 2014
@benfitzpatrick
Copy link
Contributor Author

There is still the murky problem of people writing future-trigger-chains like this:

[[[T00/PT1H]]]
    graph = "baz[^T00] => wibble"
[[[T00]]]
    graph = "bar[+P1D] => baz"
[[[T00+P1D]]]
    graph = "foo[+P1W] => bar"

where the dependency on bar will raise the effective runahead limit
to the next T00, but will not then re-raise it to incorporate bar's
dependence on foo from the next week... I'm OK with asking the
user to set their own runahead limit in this case!

else:
return base_point + interval
return ISO8601Point(str(
abbrev_util.parse_timepoint(
Copy link
Member

Choose a reason for hiding this comment

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

Should be SuiteSpecifics.abbrev_util?

@hjoliver
Copy link
Member

hjoliver commented Aug 1, 2014

with statements like foo[20T-P2W] => bar, it is no longer possible to figure out a cleanup cutoff ...that is universally valid for all points on the sequence

Can you explain this?

@benfitzpatrick
Copy link
Contributor Author

What I really mean is that, in config.py:

        for left in left_nodes:
            # (GraphNodeError checked above)
            cycle_point = None
            lnode = graphnode(left, base_interval=base_interval)
            ltaskdef = self.taskdefs[lnode.name]

            if lnode.intercycle:
                ltaskdef.intercycle = True
                if (ltaskdef.max_intercycle_offset is None or
                        lnode.offset > ltaskdef.max_intercycle_offset):
                    ltaskdef.max_intercycle_offset = lnode.offset

and in taskdef.py:

                    sself.cleanup_cutoff = (
                        sself.point + sself.max_intercycle_offset)

can't be done with varying (irregular) offsets like 20T-P2W.

To get the maximum intercycle offset for a taskdef, you might well
have to calculate the offsets for each point in the sequence (which
might be unbounded) - even then, you'd get a cleanup cutoff that
would be too large for some tasks.

For example, the foo[20T-P2W] can lead to sometimes-future-trigger
offsets and a different maximum depending on the number of days
in the relevant month.

bar foo[20T-P2W] intercycle offset
2009-12-30 2010-01-06 +P7D
2009-12-31 2010-01-06 +P6D
2010-01-01 2010-01-06 +P5D
2010-01-02 2010-01-06 +P4D
2010-01-03 2010-01-06 +P3D
2010-01-04 2010-01-06 +P2D
2010-01-05 2010-01-06 +P1D
2010-01-06 2010-01-06 +P0D
2010-01-07 2010-01-06 -P1D
2010-01-08 2010-01-06 -P2D
2010-01-09 2010-01-06 -P3D
2010-01-10 2010-01-06 -P4D
2010-01-11 2010-01-06 -P5D
2010-01-12 2010-01-06 -P6D
2010-01-13 2010-01-06 -P7D
2010-01-14 2010-01-06 -P8D
2010-01-15 2010-01-06 -P9D
2010-01-16 2010-01-06 -P10D
2010-01-17 2010-01-06 -P11D
2010-01-18 2010-01-06 -P12D
2010-01-19 2010-01-06 -P13D
2010-01-20 2010-01-06 -P14D
2010-01-21 2010-02-06 -P15D
2010-01-22 2010-02-06 -P16D

@hjoliver
Copy link
Member

hjoliver commented Aug 1, 2014

Ah, ok, thanks. I had not read very carefully and was interpreting [20T-P2W] as [P20T-P2W], which is just arithmetic to get a normal static interval. [edit: except that my version seems not to be valid anyway]

@hjoliver
Copy link
Member

hjoliver commented Aug 1, 2014

I think this is ok, but I haven't fully understood it yet (and it's getting late here now...). Is there a plausible use case for this sort of offset though, other than "if it's possible someone will try it".

@benfitzpatrick
Copy link
Contributor Author

I think @arjclark knows of a climate suite that wants "01T-P1D" to get the last day of the month

dependent_point = sequence.get_start_point()

matching_dependent_points = []
while dependent_point is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This loop worries me - it will typically start from suite initial cycle point and iterate the sequence all the way to my_point plus offset, so for a very long running suite this will get slower and slower won't it? I tested this by restarting a small suite with an irregular offset after editing the state file to make it look as if it had been running for years. Is it possible to jump onto the sequence close to my_point and go from there?

@hjoliver
Copy link
Member

hjoliver commented Aug 2, 2014

it will typically start from suite initial cycle point and iterate the sequence all the way to my_point

Actually, we're doing this kind of thing in a number places, no? In the old cyclers I was able to compute the difference between my_point and the sequence-start-point, and if the result was not a multiple of the sequence interval, adjust my_point by the remainder to get to the on-sequence point nearest to my_point. Is this not possible in the general ISO8601 case?

@benfitzpatrick
Copy link
Contributor Author

I think it is probably possible - I'll try and think of a way to do it!

@hjoliver
Copy link
Member

hjoliver commented Aug 4, 2014

I think it is probably possible - I'll try and think of a way to do it!

Sweet - I'll merge (as the task-specific calculation clearly is otherwise necessary to handle irregular intervals), and raise a new issue for this.

hjoliver added a commit that referenced this pull request Aug 4, 2014
@hjoliver hjoliver merged commit 363f76c into cylc:119.iso8601-cycling Aug 4, 2014
@benfitzpatrick benfitzpatrick deleted the 976.advanced-dependencies branch August 4, 2014 08:09
@arjclark
Copy link
Contributor

arjclark commented Aug 4, 2014

@benfitzpatrick @hjoliver:

I think @arjclark knows of a climate suite that wants "01T-P1D" to get the last day of the month

I do indeed, and I can see this behaviour becoming quite important for them.

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