Fix hang in graph with absolute point as offset #2088

matthewrmshin · 2016-12-16T10:21:01Z

Improve cutoff point for absolute offset.

Use stop point of sequence as cutoff point.

Improve error message for bad recurrence syntax
Allow long date-time format in graph offset.
Document performance issue with immortal fixed cycle point tasks.

Fix #1394 (and #1951).

matthewrmshin · 2016-12-16T10:21:49Z

lib/cylc/cycling/__init__.py

@@ -127,6 +127,8 @@ def __str__(self):
        # Stringify.
        return self.value

+    __repr__ = __str__


Friendlier display in print statements.

matthewrmshin · 2016-12-16T10:23:24Z

lib/cylc/taskdef.py

                if target_point == my_point:
                    # We have found a dependent_point for my_point.
-                    matching_dependent_points.append(dependent_point)
+                    my_cutoff_point = dependent_point


Can't see why we need an array when only its last value is used later.

oliver-sanders · 2016-12-16T10:34:12Z

The example suite in #1394 will stall if run as the start task is not retained in the pool.

oliver-sanders · 2016-12-16T10:43:18Z

Needs a test for ISO8601 absolute points. The following suite fails validation with 'NoneType' object has no attribute 'set_time_zone' due to the foo[20000102T00Z] bit.

[cylc]
    UTC mode = True
[scheduling]
    initial cycle point = 20000101T00Z
    [[dependencies]]
        [[[R1]]]
            graph = start
        [[[T00]]]
            graph = start[^] => foo
        [[[20000102T12Z]]]
            graph = foo[20000102T00Z] => bar

matthewrmshin · 2016-12-16T10:59:41Z

The example suite in #1394 will stall if run as the start task is not retained in the pool.

(Looks like we have discovered even more badness in the code.)

arjclark · 2016-12-16T12:11:31Z

tests/validate/59-absolute-as-offset.t

+__SUITE_RC__
+
+run_ok "${TEST_NAME_BASE}-datetime" cylc validate 'suite.rc'
+


Test should actually run the suite to ensure it is working (as opposed to just bypassing a validation issue)

matthewrmshin · 2016-12-16T12:30:25Z

lib/cylc/cycling/__init__.py

-        if not other:
-            return False
+        if other is None:
+            return -1


Returning False would be equivalent to returning 0 - self == other if other is None, etc.

matthewrmshin · 2016-12-16T12:31:38Z

Now running the previously bad suites, instead of just validating them.

matthewrmshin · 2016-12-16T14:14:53Z

The 'NoneType' object has no attribute 'set_time_zone' problem was a configuration syntax error. The 20000102T12Z section needs to be R1/20000102T12Z. I have added a better error message for this.

matthewrmshin · 2016-12-16T14:41:37Z

Should all be good now.

matthewrmshin · 2016-12-16T16:27:35Z

(See also #1773.)

hjoliver

Just a couple of minor questions.

hjoliver · 2016-12-19T04:05:13Z

doc/cug.tex

+            graph = "baz[20200101] => qux"
+\end{lstlisting}
+\lstset{language=transcript}
+


We should explain why this is BAD, because having a small number of immortal task proxies doesn't seem like it should be so terrible. I presume you mean because iteration across the ever-increasing range of cycle points can become a performance issue? Even so, is it worse in that respect than baz[^] => qux in a cycling section, which I see quite a lot?

Yes, this is the same as baz[^] => qux in a cycling section (see #1773). I'll explain the performance issue in the docs for now.

hjoliver · 2016-12-19T04:06:58Z

lib/cylc/config.py

+                    self.cfg['scheduling']['final cycle point'])
+            except (AttributeError, TypeError):
+                raise SuiteConfigError(
+                    "ERROR: Invalid recurrence representation: %s" % section)


Hmmm, do we know for a fact that only bad recurrence expressions cause AttributeError and TypeError here?

Looking at the statement in the try: block above, the argument to the get_sequence function are the section title (which should be a cycle point recurrence), the initial cycle point and the final cycle point.

The initial and final cycle points should already have their syntaxes checked earlier, so the only thing that can cause exception here should be invalid or unsupported recurrence syntax, or a bug in our system.

hjoliver · 2016-12-19T06:49:24Z

(tests as working, btw).

oliver-sanders · 2016-12-19T09:14:18Z

Note this will currently fail for some of the more exotic ISO8601 datetime formats:

2000-01-01T0000
20000101T00:00
2000-W01-01T0000
2000-001T0000
20000101T00,0
20000101T00.0
20000101T0000-00:00

hjoliver · 2016-12-19T09:20:17Z

It should presumably accept the configured cycle point format of the suite, at the least.

matthewrmshin · 2016-12-19T09:48:11Z

Maybe even more badness in the date-time parsing logic?

hjoliver · 2016-12-19T09:50:02Z

Can you consult with Ben over morning tea? 😄

matthewrmshin · 2016-12-19T10:06:00Z

Actually, I cannot see how any of the above can work.

2000-01-01T0000
- mixed format - long format for date and short format for time
20000101T00:00
- mixed format - short format for date and long format for time
2000-W01-01T0000
- syntax error: day of week should be single digit
2000-001T0000
- mixed format
20000101T00,0 and 20000101T00.0
- any usage for these?
20000101T0000-00:00
- mixed format - time zone in long format, the rest in short format

Note: section header appears to be OK with long format or short format, but not both. For offset, the regular expression appears to only support the short format - as the colon : character is not included. I'll see if we have any badness for supporting the colon character in offset, but maybe this is better for a different issue?

hjoliver · 2016-12-19T10:09:30Z

I guess those formats were a bit too exotic 😬

matthewrmshin · 2016-12-19T11:09:16Z

I have now added the colon : character to the regular expression for offset. It does not appear to cause any badness yet.

Improve cutoff point for absolute offset. * Use stop point of sequence as cutoff point. Improve error message for bad recurrence syntax Allow long date-time format in graph offset. Document performance issue with immortal fixed cycle point tasks.

matthewrmshin · 2016-12-19T11:27:08Z

Branch squashed and rebased.

arjclark · 2016-12-19T12:36:25Z

@oliver-sanders - given all your recent testing etc. of this can you do the review 2 of this please.

oliver-sanders

The addition of the : colon to the regex satisfies my ISO8601 concerns.

Looks OK, passes tests.

matthewrmshin added the bug Something is wrong :( label Dec 16, 2016

matthewrmshin added this to the next release milestone Dec 16, 2016

matthewrmshin self-assigned this Dec 16, 2016

matthewrmshin requested review from hjoliver and arjclark December 16, 2016 10:21

matthewrmshin commented Dec 16, 2016

View reviewed changes

arjclark reviewed Dec 16, 2016

View reviewed changes

matthewrmshin force-pushed the fix-graph-absolute-as-offset-hang branch from c0ff271 to 508f862 Compare December 16, 2016 12:28

matthewrmshin commented Dec 16, 2016

View reviewed changes

hjoliver reviewed Dec 19, 2016

View reviewed changes

matthewrmshin force-pushed the fix-graph-absolute-as-offset-hang branch from 81122a2 to 91233ac Compare December 19, 2016 11:05

matthewrmshin force-pushed the fix-graph-absolute-as-offset-hang branch from 91233ac to 7f2762a Compare December 19, 2016 11:26

arjclark requested a review from oliver-sanders December 19, 2016 12:35

oliver-sanders approved these changes Dec 19, 2016

View reviewed changes

hjoliver approved these changes Dec 20, 2016

View reviewed changes

hjoliver merged commit 0e322b8 into cylc:master Dec 20, 2016

matthewrmshin deleted the fix-graph-absolute-as-offset-hang branch January 4, 2017 15:52

matthewrmshin mentioned this pull request Jun 22, 2017

cylc gui: immortal R1 tasks: can hang obtaining graph edges #1773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hang in graph with absolute point as offset #2088

Fix hang in graph with absolute point as offset #2088

matthewrmshin commented Dec 16, 2016 •

edited

Loading

matthewrmshin Dec 16, 2016

matthewrmshin Dec 16, 2016

oliver-sanders commented Dec 16, 2016

oliver-sanders commented Dec 16, 2016 •

edited

Loading

matthewrmshin commented Dec 16, 2016

arjclark Dec 16, 2016 •

edited

Loading

matthewrmshin Dec 16, 2016

matthewrmshin commented Dec 16, 2016

matthewrmshin commented Dec 16, 2016 •

edited

Loading

matthewrmshin commented Dec 16, 2016

matthewrmshin commented Dec 16, 2016

hjoliver left a comment

hjoliver Dec 19, 2016

matthewrmshin Dec 19, 2016

hjoliver Dec 19, 2016

matthewrmshin Dec 19, 2016

hjoliver commented Dec 19, 2016

oliver-sanders commented Dec 19, 2016

hjoliver commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

hjoliver commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

hjoliver commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

arjclark commented Dec 19, 2016

oliver-sanders left a comment •

edited

Loading

		__SUITE_RC__

		run_ok "${TEST_NAME_BASE}-datetime" cylc validate 'suite.rc'

Fix hang in graph with absolute point as offset #2088

Fix hang in graph with absolute point as offset #2088

Conversation

matthewrmshin commented Dec 16, 2016 • edited Loading

matthewrmshin Dec 16, 2016

Choose a reason for hiding this comment

matthewrmshin Dec 16, 2016

Choose a reason for hiding this comment

oliver-sanders commented Dec 16, 2016

oliver-sanders commented Dec 16, 2016 • edited Loading

matthewrmshin commented Dec 16, 2016

arjclark Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

matthewrmshin Dec 16, 2016

Choose a reason for hiding this comment

matthewrmshin commented Dec 16, 2016

matthewrmshin commented Dec 16, 2016 • edited Loading

matthewrmshin commented Dec 16, 2016

matthewrmshin commented Dec 16, 2016

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver Dec 19, 2016

Choose a reason for hiding this comment

matthewrmshin Dec 19, 2016

Choose a reason for hiding this comment

hjoliver Dec 19, 2016

Choose a reason for hiding this comment

matthewrmshin Dec 19, 2016

Choose a reason for hiding this comment

hjoliver commented Dec 19, 2016

oliver-sanders commented Dec 19, 2016

hjoliver commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

hjoliver commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

hjoliver commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

matthewrmshin commented Dec 19, 2016

arjclark commented Dec 19, 2016

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

matthewrmshin commented Dec 16, 2016 •

edited

Loading

oliver-sanders commented Dec 16, 2016 •

edited

Loading

arjclark Dec 16, 2016 •

edited

Loading

matthewrmshin commented Dec 16, 2016 •

edited

Loading

oliver-sanders left a comment •

edited

Loading