Init remote via multiprocessing pool #2468

matthewrmshin · 2017-11-02T11:34:03Z

Supersede #2400.

Introduce new commands to initialise/tidy suite directory hierarchy for remote task runs. These are launched on a remote owner@host using the cylc.remote remote run mechanism. (Should tick a box in #2302.) We'll now have a remote initialisation command for each (host, owner) combo, which runs using via the multi-processing pool. Submission of tasks of a (host, owner) combo are deferred until the command completes, with no hold-up to the main loop.

Commands to select the remote host for tasks are also launched via the multi-processing pool. If multiple ready tasks have the same host select command string, the command will only be run once for the current batch of tasks. When the ready tasks have consumed the results, the logic will reset the results. This change will allow the main loop to be responsive while host select commands are running.

Update log and runtime database only before the jobs-submit command is launched.
Cleaner separation between job file write logic and job submission logic. (Still more to do, however.)

~~Address host initialisation/preparation part of #2292. (Host selection part of #2292 likely to be considered/superseded by #2199.)~~

Close #2292.

~~TO BE FULLY SITE TESTED.~~

matthewrmshin · 2017-11-02T12:28:14Z

(The branch is the same as the original in #2400, but re-based against latest master.)

hjoliver · 2017-11-05T03:40:24Z

(conflicts)

matthewrmshin · 2017-11-06T08:48:12Z

Re-based.

hjoliver

Looks good, tests as working.

hjoliver · 2017-11-06T23:10:50Z

Are you still intending to do this: #2400 (review) ? (maybe it's not necessary to, essentially, check that pool commands really do happen in the background).

matthewrmshin · 2017-11-07T10:28:31Z

(I am having another look at the logic, and I think something is not quite right, so please hold off reviewing for now.)

matthewrmshin · 2017-11-08T20:26:50Z

@hjoliver A question for you. For historical reason, the remote initialisation logic copies the python/ sub-directory of the suite to the remote task host. The use of the python/ sub-directory is apparently deprecated according to the documentation. Should we continue to copy this sub-directory to the remote task host? Or should we be copying lib/python/ (and bin/ and other stuffs) instead?

hjoliver · 2017-11-08T20:40:57Z

@matthewrmshin good question. As I recall, the deprecation was to do with what is automatically added to sys.path in the scheduler. I don't recall what our policy is, if anything, (I can't look right now), on what is installed from suite dir to job hosts? But without some way of configuring that (migration of rose suite-run will solve this I guess) I suppose we should either copy nothing (i.e. it is up to the user) or everything?

matthewrmshin · 2017-11-11T09:45:40Z

(The Codacy issues are nonsense in the context of this change.)

matthewrmshin · 2017-11-13T12:20:42Z

@hjoliver This is now ready for another look.

matthewrmshin · 2017-11-13T14:12:31Z

(Finally got Codacy to not complain.)

hjoliver · 2017-11-20T04:50:19Z

I'm hoping to re-review this tomorrow (Tuesday).

matthewrmshin · 2018-01-05T10:20:34Z

Branch re-based and de-conflicted. Codacy failure is not applicable in this case - the logic has been moved from one module to a new one - and STDIN is already being redirected from /dev/null.

New commands to initialise/tidy suite directory hierarchy for remote task runs. Commands now run in the background, and launched on remote hosts as using `cylc.remote` remote run mechanism. (Tick a box in cylc#2302.) Update log and runtime database only before the real submission command.

Task host select commands are now done via the process pool. If multiple ready tasks have the same host select command string, the command will only be run once for the current batch of tasks. When the ready tasks have consumed the results, the logic will reset the results. This change will allow the main loop to be responsive while host select commands are running.

matthewrmshin · 2018-01-09T08:59:07Z

(Again, the Codacy issues are nonsense in the context of this change. The problem logic is just moved from an old module to a new module.)

hjoliver

This looks pretty good, but I still need to do some testing (tomorrow)

hjoliver · 2018-01-09T10:10:22Z

bin/cylc-help

@@ -422,6 +424,8 @@ comsum['broadcast'] = 'Change suite [runtime] settings on the fly'
 comsum['jobs-kill'] = '(Internal) Kill task jobs'
 comsum['jobs-poll'] = '(Internal) Retrieve status for task jobs'
 comsum['jobs-submit'] = '(Internal) Submit task jobs'
+comsum['remote-init'] = '(Internal) Initialise a task remote'
+comsum['remote-tidy'] = '(Internal) Tidy a task remote'


We should document this terminology, which I like, and which I guess comes from task [[[remote]]], and use it consistently - perhaps in another PR though.

I've mostly been using the uglier "task job host account" or similar, I think. Something like this?: a "task remote" is a user account, other than the suite host account, where a task job is submitted to run. It can be on the suite host machine or another machine.

Can we do this in a follow-on PR? (This PR has lasted long enough for more changes.)

(The [user@]host syntax is sometimes called an authority - see https://en.wikipedia.org/wiki/URL for example - but everyone are confused by the phrase.)

Yes, that's fine - and sorry for the review delay on this!

Re-submit only waiting tasks.

oliver-sanders · 2017-11-17T15:47:43Z

bin/cylc-submit

-        task_job_mgr.prep_submit_task_jobs(suite, itasks, dry_run=True)
+        while waiting_tasks:
+            prep_tasks, bad_tasks = task_job_mgr.prep_submit_task_jobs(
+                suite, itasks, dry_run=True)


should this be waiting_tasks rather than itasks?

oliver-sanders · 2018-01-10T09:44:24Z

lib/cylc/mp_pool.py

+            if len(ctx.cmd_kwargs['stdin_file_paths']) > 1:
+                stdin_file = TemporaryFile()
+                for file_path in ctx.cmd_kwargs['stdin_file_paths']:
+                    stdin_file.write(open(file_path, 'rb').read())


If these stdin files are likely to be large it might be worth writing line by line to avoid loading the whole file into memory.

I think the main usage here is job files, so no likely to be big enough to cause issues.

oliver-sanders · 2018-01-10T11:50:02Z

bin/cylc-submit

@@ -126,7 +136,12 @@ def main():
                    'Unable to prepare job file for %s' % itask.identity)
                ret_code = 1
    else:
-        task_job_mgr.submit_task_jobs(suite, itasks)
+        while waiting_tasks:
+            for itask in task_job_mgr.submit_task_jobs(suite, itasks):


should this be waiting_tasks rather than itasks?

oliver-sanders · 2018-01-11T12:40:40Z

lib/cylc/task_remote_mgr.py

+                        'remote-host-select', cmd, env=dict(os.environ)),
+                    self._remote_host_select_callback, [cmd_str])
+                self.remote_host_str_map[cmd_str] = None
+                return self.remote_host_str_map[cmd_str]


Should this return here?

If called for the first time the is_remote_host logic is bypassed but if the result is cashed the logic is run?

Yes, returning None here. The command has only been put into the process pool, so we'll have to wait for it to return.

The follow-on logic is only run for a straightforward host string, or for a host string returned by a successful/cached host select command.

oliver-sanders · 2018-01-11T15:46:30Z

lib/cylc/task_remote_mgr.py

+            pass  # Not yet initialised
+        else:
+            if status == REMOTE_INIT_FAILED:
+                del self.remote_init_map[(host, owner)]  # reset to allow retry


Do we need a mechanism to prevent the potential for an infinite loop of failed remote_init attempts.

Yes, we should add this functionality when we tackle #2315.

oliver-sanders · 2018-01-11T15:48:51Z

lib/cylc/task_remote_mgr.py

+            self.suite, 'suite run directory', host, owner))
+        self.proc_pool.put_command(
+            SuiteProcContext(
+                'remote-init', cmd, stdin_file_paths=[tmphandle.name]),


Would it be possible to scrap the need for tar files and do something like stdin_file_paths=[path for path, _ in items] at this end (maybe | xargs on the other side ...)?

No, I think usage of a TAR file is the cleanest implementation. It allows us to transfer a whole set of files (file names, modes, potentially binary contents, etc) via a single remote command on a single SSH session. #2302 demands that we don't use a shell on the remote side.

oliver-sanders · 2018-01-11T16:29:56Z

lib/cylc/task_remote_mgr.py

+        items = []
+        comm_meth = GLOBAL_CFG.get_host_item(
+            'task communication method', host, owner)
+        LOG.debug('comm_meth=%s' % comm_meth)


Is this needed any more?

OK for debug purpose. I am sure we'll revisit this when we tackle #2214.

oliver-sanders

Hunky dory!

matthewrmshin added the efficiency For notable efficiency improvements label Nov 2, 2017

matthewrmshin added this to the next release milestone Nov 2, 2017

matthewrmshin self-assigned this Nov 2, 2017

matthewrmshin mentioned this pull request Nov 2, 2017

Host initialisation refactor to use multiprocess pool #2400

Closed

matthewrmshin requested a review from hjoliver November 2, 2017 12:27

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from 75615d7 to a9eea1a Compare November 3, 2017 09:06

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from a9eea1a to b619620 Compare November 6, 2017 08:47

hjoliver approved these changes Nov 6, 2017

View reviewed changes

hjoliver requested a review from oliver-sanders November 6, 2017 23:07

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from b619620 to 8d61b30 Compare November 8, 2017 15:48

matthewrmshin changed the title ~~Init host refactor use multiproc pool~~ Init remote via multiprocessing pool Nov 8, 2017

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from 8d61b30 to e19b4e8 Compare November 9, 2017 11:26

matthewrmshin mentioned this pull request Nov 9, 2017

Set default task comms to client server comms. #2479

Merged

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch 6 times, most recently from 0bd8979 to de79487 Compare November 11, 2017 07:44

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from de79487 to 9b00e55 Compare November 13, 2017 10:33

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from 9b00e55 to abe4776 Compare November 13, 2017 12:42

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from abe4776 to 106e8ed Compare November 13, 2017 12:51

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from 106e8ed to 8a64c7f Compare November 14, 2017 10:28

hjoliver mentioned this pull request Nov 16, 2017

Use cylc subcommand for remote functionality #2302

Closed

5 tasks

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from 8a64c7f to 0d35bbb Compare November 24, 2017 20:58

matthewrmshin mentioned this pull request Dec 6, 2017

slurm --clusters support #2504

Open

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from 0d35bbb to 62f018f Compare January 5, 2018 09:39

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from cdb0b50 to bd50196 Compare January 8, 2018 09:16

dvalters and others added 5 commits January 9, 2018 08:51

Happy new year to new files

ca41c66

remun usage fix

4e93e26

Redirect Popen stdin from /dev/null in new module

a6fc760

matthewrmshin force-pushed the init-host-refactor-use-multiproc-pool branch from bd50196 to a6fc760 Compare January 9, 2018 08:51

hjoliver reviewed Jan 9, 2018

View reviewed changes

hjoliver approved these changes Jan 10, 2018

View reviewed changes

cylc submit: minor logic improvement

8020094

Re-submit only waiting tasks.

hjoliver mentioned this pull request Jan 10, 2018

document "task remote" terminology #2537

Closed

oliver-sanders reviewed Jan 11, 2018

View reviewed changes

oliver-sanders mentioned this pull request Jan 12, 2018

timeout for external commands? #2315

Closed

oliver-sanders approved these changes Jan 12, 2018

View reviewed changes

oliver-sanders merged commit 5add31e into cylc:master Jan 12, 2018

matthewrmshin deleted the init-host-refactor-use-multiproc-pool branch January 12, 2018 15:17

matthewrmshin mentioned this pull request Jan 12, 2018

Fix check-versions and other tests #2539

Merged

Init remote via multiprocessing pool #2468

Init remote via multiprocessing pool #2468

Conversation

matthewrmshin commented Nov 2, 2017 • edited Loading

matthewrmshin commented Nov 2, 2017

hjoliver commented Nov 5, 2017

matthewrmshin commented Nov 6, 2017

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented Nov 6, 2017

matthewrmshin commented Nov 7, 2017

matthewrmshin commented Nov 8, 2017

hjoliver commented Nov 8, 2017

matthewrmshin commented Nov 11, 2017

matthewrmshin commented Nov 13, 2017

matthewrmshin commented Nov 13, 2017

hjoliver commented Nov 20, 2017

matthewrmshin commented Jan 5, 2018

matthewrmshin commented Jan 9, 2018

hjoliver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrmshin Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrmshin Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

matthewrmshin commented Nov 2, 2017 •

edited

Loading

matthewrmshin Jan 11, 2018 •

edited

Loading

matthewrmshin Jan 11, 2018 •

edited

Loading