Task state code refactor. #1775

hjoliver · 2016-04-01T05:43:08Z

Refactor the code associated with handling of task state/status.

[updated 13 April - original lines ~~crossed out~~]

Motivation: working on another current pull request, I was bitten by a typo in a task status string comparison, which wouldn't happen if we used (constant) variables to refer to states like "waiting" etc. (which would be better for internationalization too, if it comes to that at some point...). Further, each task proxy currently holds its status in a "task_state" object that is (a) next to useless - it contains just a status string, and some minimal logic that is largely duplicated in wrapper methods in the task proxy class anyway; and (b) also holds "spawned/not spawned" status in a silly two-key dict along with "status", which doesn't make much sense. This is all dates back a long way, before I learned a few things off @cylc/core members 😬

So: this PR does the following:

separates spawning from task_state
deprecates "cylc reset --state=spawn" to a new command "cylc spawn"
~~gets rid of the task state objects (now just a status string held by task proxies)~~
the task_state module now defines a TaskState class that (for each task proxy) holds the simple status string, and prerequisites and outputs (which are intimately connected to task status), plus associated logic.
the task state module also defines constants for each status string, e.g. STATUS_WAITING = 'waiting', and these are now used throughout the code.
replaces the state.is_currently() and state.get_status() methods with static set membership tests, for performance reasons (as pointed out by @benfitzpatrick below).

@matthewrmshin - this isn't quite ready - I still need to use STATUS_WAITING etc. throughout the code instead of hard-coded strings "waiting" etc.; add a test for "cylc spawn", and address a few TODO comments left in the code (mainly for checking valid status at various points). However, as this grew a bit bigger than I was expecting, I just wanted to post the branch as-is before the weekend in case of any fundamental objections, or suggestions for improvement. Test battery passes on this branch at the moment.

arjclark · 2016-04-01T07:48:12Z

+1 from me for moving from strings to constants (e.g. state.WAITING_STATE over 'waiting') - pretty much for the typos reason you give above

benfitzpatrick · 2016-04-07T13:59:43Z

This would be good for performance - is_currently and get_status are pretty slow according to the profiler (could be profile overhead, could be Python function call overhead).

hjoliver · 2016-04-07T23:31:53Z

I'll get rid of is_currently and get_status.


(almost done - tests passing, just pep8 and some tidying to do)

hjoliver · 2016-04-13T03:32:10Z

@matthewrmshin - this is ready to review, test battery passes here. Description updated above. (note I backed out of a more extensive refactoring of task_proxy module, will email re this later...).

(I do intend to add some more docstrings yet)

matthewrmshin · 2016-04-13T10:29:48Z

Test battery OK (with and without global.rc) with the exception the PEP8 test, which is failing with something like this in my environment:

-/home/matt/cylc.git/lib/cylc/task_pool.py:276:20: E713 test for membership should be 'not in'
-/home/matt/cylc.git/lib/cylc/task_pool.py:1485:21: E713 test for membership should be 'not in'
-/home/matt/cylc.git/lib/cylc/task_pool.py:1491:21: E713 test for membership should be 'not in'
-/home/matt/cylc.git/lib/cylc/task_proxy.py:1379:17: E713 test for membership should be 'not in'
-/home/matt/cylc.git/lib/cylc/task_state.py:356:16: E713 test for membership should be 'not in'
-/home/matt/cylc.git/bin/cylc-restart:483:20: E713 test for membership should be 'not in'

matthewrmshin · 2016-04-13T15:09:16Z

(I'll attempt to look at this change in detail tomorrow.)

hjoliver · 2016-04-13T21:50:46Z

note I backed out of a more extensive refactoring of task_proxy module, ~~will email re this later~~...

The handling of state, prerequisites, outputs, retries, incoming messages, events, DB writes, etc., in the task_proxy module has become increasingly detailed and unnecessarily tangled, and IMO the whole thing could use a complete redesign, to abstract out the messy detail. I actually made a start on it in this branch (because moving prereqs and outputs to task_state.py goes a little way toward that goal) but I backed out of it pretty quickly because it could be a lot of work and a difficult change to review, and for no functional purpose at this point. However, I thought I'd just flag that this should probably be done at some point in the future...

matthewrmshin · 2016-04-14T13:32:56Z

bin/cylc-spawn

+
+    suite = args.pop(0)
+
+    prompt('Reset task(s) %s in %s' % (args, suite), options.force)


Spawn instead of Reset?

hjoliver · 2016-04-19T03:10:09Z

Rebased.

matthewrmshin · 2016-04-19T11:42:32Z

Branch needs rebase again, I am afraid.

matthewrmshin · 2016-04-19T11:51:42Z

lib/cylc/gui/app_gcylc.py

-            self.legal_task_states = TaskState.legal_for_restricted_monitoring
-        else:
-            self.legal_task_states = TaskState.legal
+            self.legal_task_states = TASK_STATUSES_RESTRICTED


I think self.legal_task_states needs to be set regardless. I am getting a traceback on View > Task Filtering.

Traceback (most recent call last): File "/home/matt/cylc.git/lib/cylc/gui/app_gcylc.py", line 3150, in popup_filter_dialog self.create_task_filter_widgets() File "/home/matt/cylc.git/lib/cylc/gui/app_gcylc.py", line 2911, in create_task_filter_widgets n_states = len(self.legal_task_states) AttributeError: 'ControlApp' object has no attribute 'legal_task_states'

Good spotting, I should have tested that.

hjoliver · 2016-04-19T22:15:24Z

Branch rebased, bug fixed.

A minor thing I forget to mention above - this PR also makes the gcylc task right-click View menu a bit more context sensitive: for submitted, submit-failed, and submit-retrying tasks you can view job script and activity log, but not job logs.

hjoliver · 2016-04-28T00:01:37Z

Branched rebased, tests passing.

benfitzpatrick · 2016-04-29T11:17:52Z

Sorry for conflicting this one up!

hjoliver · 2016-04-30T06:22:47Z

Sorry for conflicting this one up!

Again!

But never mind, it's worth it!

matthewrmshin · 2016-05-06T11:47:11Z

@hjoliver Sorry, but as usual, this branch is in conflicts again. We should aim to merge this in as soon as possible so that we can be testing with it in for most of the new release cycle.

hjoliver · 2016-05-08T22:55:10Z

(I'll sort out the conflicts as soon as possible...)

Fix rebase error.

hjoliver · 2016-05-10T00:35:09Z

@matthewrmshin - branch rebased, tests pass here. It might be a good idea for @benfitzpatrick to take a quick look at this too, to check my resolution of the conflicts with his changes #1813 and #1819.

matthewrmshin · 2016-05-10T09:49:55Z

Looks OK. Tests OK (with and without global configuration) in my environment.

@benfitzpatrick please review 2.

benfitzpatrick · 2016-05-11T09:16:02Z

bin/cylc-restart

@@ -333,7 +337,8 @@ class restart(Scheduler):
        for line in task_lines:
            # instance variables
            try:
-                (id_, state) = line.split(' : ')
+                (id_, state_string) = line.split(' : ')
+                # state_string e.g. 'status=running, spawned=false'


benfitzpatrick · 2016-05-12T14:32:52Z

Looks like it leads to a speedup! Great!

hjoliver self-assigned this Apr 1, 2016

hjoliver added this to the soon milestone Apr 1, 2016

hjoliver force-pushed the task-state-refactor branch 8 times, most recently from afc796f to dc9774e Compare April 12, 2016 06:43

hjoliver assigned matthewrmshin and unassigned hjoliver Apr 13, 2016

hjoliver changed the title ~~Task state code refactor (in progress...)~~ Task state code refactor. Apr 13, 2016

matthewrmshin reviewed Apr 14, 2016
View reviewed changes

This was referenced Apr 14, 2016

Task proxy module refactor. #1794

Closed

Fix polling of jobs submitted to Loadleveler. #1762

Merged

hjoliver force-pushed the task-state-refactor branch from 5a26daa to f955d79 Compare April 19, 2016 03:09

hjoliver mentioned this pull request Apr 19, 2016

Proposed new task polling logic. #1792

Closed

matthewrmshin reviewed Apr 19, 2016
View reviewed changes

hjoliver force-pushed the task-state-refactor branch from f955d79 to 38e69f7 Compare April 19, 2016 21:48

hjoliver force-pushed the task-state-refactor branch from 707f70d to c42c7ef Compare April 27, 2016 23:57

hjoliver modified the milestones: next release, soon May 4, 2016

hjoliver force-pushed the master branch 3 times, most recently from 17fe934 to 3bdb067 Compare May 5, 2016 01:37

hjoliver added 4 commits May 10, 2016 11:45

Task state code refactor.

78166a2

Review feedback (typo fix).

bf9ee06

Fix GUI (review feedback).

4ef9286

Fix GUI right-click View items sensitivity.

fe47835

Fix rebase error.

hjoliver force-pushed the task-state-refactor branch from c42c7ef to fe47835 Compare May 9, 2016 23:56

matthewrmshin assigned benfitzpatrick and unassigned matthewrmshin May 10, 2016

benfitzpatrick reviewed May 11, 2016
View reviewed changes

Fixed comment.

fc3d38f

arjclark mentioned this pull request May 12, 2016

suite stalled warning or notification #1286

Closed

benfitzpatrick merged commit 2cebf3c into cylc:master May 12, 2016

arjclark added a commit to arjclark/cylc that referenced this pull request May 13, 2016

Resolve confilicts and update code following cylc#1775

3b654e9

arjclark mentioned this pull request May 13, 2016

Add ability to configure initial gcylc filter settings. #1833

Merged

benfitzpatrick mentioned this pull request May 24, 2016

cylc run --hold: prereqs caching breaks pre-initial dependencies #1857

Closed

hjoliver deleted the task-state-refactor branch June 24, 2016 13:17

MetRonnie mentioned this pull request Aug 6, 2024

TaskState: remove cached prereq satisfaction in preparation for cylc remove #6287

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task state code refactor. #1775

Task state code refactor. #1775

hjoliver commented Apr 1, 2016

arjclark commented Apr 1, 2016

benfitzpatrick commented Apr 7, 2016

hjoliver commented Apr 7, 2016

hjoliver commented Apr 13, 2016

matthewrmshin commented Apr 13, 2016

matthewrmshin commented Apr 13, 2016

hjoliver commented Apr 13, 2016

matthewrmshin Apr 14, 2016

hjoliver commented Apr 19, 2016

matthewrmshin commented Apr 19, 2016

matthewrmshin Apr 19, 2016

hjoliver Apr 19, 2016

hjoliver commented Apr 19, 2016

hjoliver commented Apr 28, 2016

benfitzpatrick commented Apr 29, 2016

hjoliver commented Apr 30, 2016

matthewrmshin commented May 6, 2016

hjoliver commented May 8, 2016

hjoliver commented May 10, 2016

matthewrmshin commented May 10, 2016

benfitzpatrick May 11, 2016

benfitzpatrick commented May 12, 2016


		suite = args.pop(0)

		prompt('Reset task(s) %s in %s' % (args, suite), options.force)

Task state code refactor. #1775

Task state code refactor. #1775

Conversation

hjoliver commented Apr 1, 2016

arjclark commented Apr 1, 2016

benfitzpatrick commented Apr 7, 2016

hjoliver commented Apr 7, 2016

hjoliver commented Apr 13, 2016

matthewrmshin commented Apr 13, 2016

matthewrmshin commented Apr 13, 2016

hjoliver commented Apr 13, 2016

matthewrmshin Apr 14, 2016

Choose a reason for hiding this comment

hjoliver commented Apr 19, 2016

matthewrmshin commented Apr 19, 2016

matthewrmshin Apr 19, 2016

Choose a reason for hiding this comment

hjoliver Apr 19, 2016

Choose a reason for hiding this comment

hjoliver commented Apr 19, 2016

hjoliver commented Apr 28, 2016

benfitzpatrick commented Apr 29, 2016

hjoliver commented Apr 30, 2016

matthewrmshin commented May 6, 2016

hjoliver commented May 8, 2016

hjoliver commented May 10, 2016

matthewrmshin commented May 10, 2016

benfitzpatrick May 11, 2016

Choose a reason for hiding this comment

benfitzpatrick commented May 12, 2016