Additions to provide support for the Load Sharing Facility (LSF) job scheduler #2373

markmcdowall · 2018-03-13T14:07:28Z

Description

Provides support for the use of the Load Sharing Facility (LSF) job scheduler for the distributed computation of tasks within a high performance cluster environment. The work is based off the SGE wrapper and work that had been done by @alexbw.

Motivation and Context

Needed this so that we could use the Luigi infrastructure on our local LSF cluster

#1936

Have you tested this? If so, how?

I have run this code as part of a local pipeline and everything ran smoothly. There is also a small unittest script

Notes

Updates were based on the work by Alex with changes so that jobs could run within the latest version of Luigi.
Pulled in the latest changes from master
Added new parameters for naming of jobs, listing queues and memory sizes.
Added in a print out to provide a rough timing for how long a bsub job ran for. This is dependent on the polling time.
The LSF wrapper has been moved into the contrib folder
Some code tidying based on pylint output
Added doc sections and copied over the Apache 2.0 license from Spotify. You'll need to add your institute into the list, I have left a gap for this already

…h and break in earnest

…hained jobs, and multiple workers

…flicts

…ome extra bsub params, timing of jobs and some clean up based on pylint

dlstadther · 2018-03-13T15:47:47Z

@markmcdowall Please address travis errors and add tests were applicable, if possible.

markmcdowall · 2018-03-13T17:30:27Z

Hi @dlstadther,

I am still getting a failure with the docs not building correctly. Would you be able to advise on why it might be failing?

Cheers,

Mark

dlstadther · 2018-03-13T17:50:40Z

/home/travis/build/spotify/luigi/luigi/contrib/lsf.py:docstring of luigi.contrib.lsf.LSFJobTask.run:5: ERROR: Unexpected indentation.

You also have some other Py3 errors

Tarrasch · 2018-03-13T20:33:52Z

@markmcdowall, Can you please update the PR title and description to be more useful for casual reaters? What's LSF? Who should care etc. Thanks. :)

markmcdowall · 2018-04-09T13:03:53Z

Hi @dlstadther,

I have addressed, or commented on, each of he issues that you raised and Travis is giving the green light.

Cheers,

Mark

dlstadther

Mind getting one other reviewer? Thanks!

markmcdowall · 2018-04-09T13:59:08Z

Hi @dlstadther ,

Thanks for approving the changes. As for the second reviewer would this need to be someone else that has already contributed to the Luigi repo? If so is there a list of people that are happy to be approached?

markmcdowall · 2018-05-09T15:24:04Z

Hi @dlstadther,

Would it be possible to add @lairdm as a reviewer?

Cheers,

Mark

dlstadther · 2018-05-09T21:10:23Z

@lairdm, could you oblige @markmcdowall 's review request? Thanks!

lairdm · 2018-06-13T09:32:05Z

luigi/contrib/lsf.py

+    - "EXIT"
+    based on the LSF documentation
+    """
+    cmd = "bjobs -noheader -o stat %d" % job_id


Should you use new style .format()? Old style hasn't been deprecated yet, but the threat is still hanging out there.

lairdm · 2018-06-13T09:49:56Z

luigi/contrib/lsf.py

+    based on the LSF documentation
+    """
+    cmd = "bjobs -noheader -o stat %d" % job_id
+    track_job_proc = subprocess.Popen(


Do you need to put this in a try block, or is it alright if things die loudly if the sub-process can't be made?

Not really as blank will get returned and handled later in the script

lairdm · 2018-06-13T09:52:20Z

luigi/contrib/lsf.py

+    """
+    Kill a running LSF job
+    """
+    subprocess.call(['bkill', job_id])


Does Luigi support returning if a kill was successful or not?

The only response would be from LSF saying that it is terminating the job with the given id. There is nothing from LSF to say if it was successful or not.

lairdm · 2018-06-13T09:55:38Z

luigi/contrib/lsf.py

+        Read in the output file
+        """
+        # Read in the output file
+        with open(os.path.join(self.tmp_dir, "job.out"), "r") as f_out:


Nitpicky, but with errors you check if the file exists, is there a reason the same logic isn't mirrored here?

lairdm · 2018-06-13T10:06:49Z

luigi/contrib/lsf.py

+        self.tmp_dir = os.path.join(base_tmp_dir, task_name)
+        # Max filename length
+        max_filename_length = os.fstatvfs(0).f_namemax
+        self.tmp_dir = self.tmp_dir[:max_filename_length]


I'm not sure I follow what you're trying to do. I'm unsure if f_namemax is the max length of a filename component or the entire path. Either way, you're truncating the base_tmp_dir + task_name concatenated path to f__namemax then adding packages.tar a bit later. If f_namemax is the max total path length, wouldn't making it longer be an issue? If it's the max length of a component of the path, shouldn't it be truncated before prepending the base path?

lairdm · 2018-06-13T10:21:46Z

luigi/contrib/lsf.py

+        args += ["-W", str(self.runtime_flag)]
+        if self.job_name_flag:
+            args += ["-J", str(self.job_name_flag)]
+        args += ["-o", "/".join(log_output[0:-1]) + "/job.out"]


If you want to drop the last component of the path, why this way rather than rsplit as with the path earlier, then us the OS safe os.path module?

lairdm · 2018-06-13T10:27:06Z

luigi/contrib/lsf.py

+            args += self.extra_bsub_args.split()
+
+        # Find where the runner file is
+        runner_path = lsf_runner.__file__


Are you sure that's what you want, depending on the configuration of sys.path that could be a relative or absolute path:
https://stackoverflow.com/a/7116925/3076561

lairdm · 2018-06-13T10:32:30Z

luigi/contrib/lsf.py

+
+    def _track_job(self):
+        time0 = 0
+        while True:


Is it ok if we get stuck in this call forever if the job never releases or is suspended?

This should only happen if jobs are hanging while in the queue to get onto LSF. If a job has been held back this could be due to the LSF manager prioritising other jobs over yours or you could have held your jobs from completing to clear a backlog of other jobs that are waiting.

… and not the whole path

…e job output file paths

markmcdowall · 2018-07-04T09:55:16Z

Hi @dlstadther ,

Sorry for the delay. I have gone through the comments made by @lairdm about the code and made relevant changes and also run a few pipelines to check that the changes did not break anything.

Are you happy with the changes that have been made to the code?

Cheers,

Mark

lairdm · 2018-07-05T10:29:52Z

luigi/contrib/lsf.py

@@ -236,8 +235,8 @@ def _run_job(self):
        args += ["-W", str(self.runtime_flag)]
        if self.job_name_flag:
            args += ["-J", str(self.job_name_flag)]
-        args += ["-o", os.path.join(log_output[0:-1], "job.out")]
-        args += ["-e", os.path.join(log_output[0:-1], "job.err")]
+        args += ["-o", os.path.join("/".join(log_output[0:-1]), "job.out")]


Sorry, why are you flipping back to "/".join()? That's not OS safe. Unless I'm missing something.

I've modified the code to use the os.path.split and os.path.join functions.

The reason for using the "/".join() is that the os.path.join could only take in strings. Using the os.path.split should get around this issue.

lairdm · 2018-07-05T10:30:42Z

Other than the "/".join() question, I'm happy. Thanks.

dlstadther

This doesn't impact core, so I'm good with this if @lairdm is (which appears to be the case)

markmcdowall · 2018-07-12T12:55:33Z

Hi @dlstadther ,

Thank you for accepting the changes. Do I need to provide any further information or is it now over to you guys?

Cheers,

Mark

@dlstadther

…scheduler (spotify#2373) * Beginning work on LSF batching * Code is all written down right from my head. Now need to test and push and break in earnest * Flags for output and error files * More thoughts on a daemon process * First (as far as I can tell) working LSF submission system * Reporting unknown status. Also, everything's nice and tested now on chained jobs, and multiple workers * Now saving directories as parameters doesn't act funny * Added a local LSF task, for debugging. * Can now pass extra args to the batch submission * An inheritance decorator * Method and practice for instantiating classes that are inherited from * Removed log * More tests, and ability to nullify parameters * Added better naming to common_params, and an assertion and test to boot * Moved the LSF modules to match the use of the contrib folder. Added some extra bsub params, timing of jobs and some clean up based on pylint * Removed extra newline * Improvements based on flake8 feedback * Import issues with dill and improvements to style * Fixed error with indentation of docs and error handling * Unit test for the LSF code * Spelling correction * Fixes to get the tests to work * Added to the unit tests * Removed unused imports * Removed unused imports * Tidied up the track_job function to remove the requirement on awk * Changes based on the feedback from @dlstadther * Removed print statement * Fix for the default value for the job_name_flag * Tests failed using the OptionalParameter, reverted back to Parameter with a default * Better handling of testing if jobs failed * Fixes based on feedback from @lairdm * Modified the shortening of the file names to the affect just the name and not the whole path * Testing the previsou shorter naming * Shortened the naming as was not the issue. Changed the creation of the job output file paths * Fix to fully use the os.path manipulation for dir location

@dlstadther

…scheduler (spotify#2373) * Beginning work on LSF batching * Code is all written down right from my head. Now need to test and push and break in earnest * Flags for output and error files * More thoughts on a daemon process * First (as far as I can tell) working LSF submission system * Reporting unknown status. Also, everything's nice and tested now on chained jobs, and multiple workers * Now saving directories as parameters doesn't act funny * Added a local LSF task, for debugging. * Can now pass extra args to the batch submission * An inheritance decorator * Method and practice for instantiating classes that are inherited from * Removed log * More tests, and ability to nullify parameters * Added better naming to common_params, and an assertion and test to boot * Moved the LSF modules to match the use of the contrib folder. Added some extra bsub params, timing of jobs and some clean up based on pylint * Removed extra newline * Improvements based on flake8 feedback * Import issues with dill and improvements to style * Fixed error with indentation of docs and error handling * Unit test for the LSF code * Spelling correction * Fixes to get the tests to work * Added to the unit tests * Removed unused imports * Removed unused imports * Tidied up the track_job function to remove the requirement on awk * Changes based on the feedback from @dlstadther * Removed print statement * Fix for the default value for the job_name_flag * Tests failed using the OptionalParameter, reverted back to Parameter with a default * Better handling of testing if jobs failed * Fixes based on feedback from @lairdm * Modified the shortening of the file names to the affect just the name and not the whole path * Testing the previsou shorter naming * Shortened the naming as was not the issue. Changed the creation of the job output file paths * Fix to fully use the os.path manipulation for dir location

* upstream-master: (82 commits) S3 client refactor (spotify#2482) Rename to rpc_log_retries, and make it apply to all the logging involved Factor log_exceptions into a configuration parameter Fix attribute forwarding for tasks with dynamic dependencies (spotify#2478) Add a visiblity level for luigi.Parameters (spotify#2278) Add support for multiple requires and inherits arguments (spotify#2475) Add metadata columns to the RDBMS contrib (spotify#2440) Fix race condition in luigi.lock.acquire_for (spotify#2357) (spotify#2477) tests: Use RunOnceTask where possible (spotify#2476) Optional TOML configs support (spotify#2457) Added default port behaviour for Redshift (spotify#2474) Add codeowners file with default and specific example (spotify#2465) Add Data Revenue to the `blogged` list (spotify#2472) Fix Scheduler.add_task to overwrite accepts_messages attribute. (spotify#2469) Use task_id comparison in Task.__eq__. (spotify#2462) Add stale config Move github templates to .github dir Fix transfer config import (spotify#2458) Additions to provide support for the Load Sharing Facility (LSF) job scheduler (spotify#2373) Version 2.7.6 ...

alexbw and others added 19 commits September 12, 2013 19:27

Beginning work on LSF batching

7d174f6

Code is all written down right from my head. Now need to test and pus…

2291144

…h and break in earnest

Flags for output and error files

fe180f9

More thoughts on a daemon process

f52fe5b

First (as far as I can tell) working LSF submission system

2885e96

Reporting unknown status. Also, everything's nice and tested now on c…

6105e55

…hained jobs, and multiple workers

Now saving directories as parameters doesn't act funny

b302ab9

Added a local LSF task, for debugging.

8a83b0d

Can now pass extra args to the batch submission

5546b88

An inheritance decorator

8627863

Merged the inherit decorator into the LSF branch

f63e42d

Method and practice for instantiating classes that are inherited from

5ddf877

Merged new tools

99ae8ba

Removed log

4ee00c5

More tests, and ability to nullify parameters

8d03933

Merge branch 'inherit_decorator' into lsf

1e8eca7

Added better naming to common_params, and an assertion and test to boot

cc0d3a0

Pulled in the latest version of master to the branch and resolved con…

df78b12

…flicts

Moved the LSF modules to match the use of the contrib folder. Added s…

cd949e6

…ome extra bsub params, timing of jobs and some clean up based on pylint

markmcdowall and others added 3 commits March 13, 2018 15:56

Removed extra newline

0ce2aa1

Improvements based on flake8 feedback

1508ca6

Import issues with dill and improvements to style

1ba2c79

markmcdowall changed the title ~~Lsf~~ Additions to provide support for the Load Sharing Facility (LSF) job scheduler Mar 14, 2018

markmcdowall added 3 commits March 14, 2018 09:26

Fixed error with indentation of docs and error handling

21ba36d

Unit test for the LSF code

428136f

Spelling correction

7dfe15f

Better handling of testing if jobs failed

4cd0153

dlstadther approved these changes Apr 9, 2018

View reviewed changes

lairdm reviewed Jun 13, 2018

View reviewed changes

markmcdowall added 5 commits July 4, 2018 08:59

Fixes based on feedback from @lairdm

79c3120

Merge branch 'lsf' of https://github.com/markmcdowall/luigi into lsf

9b5e7f3

Modified the shortening of the file names to the affect just the name…

94c5080

… and not the whole path

Testing the previsou shorter naming

cf03481

Shortened the naming as was not the issue. Changed the creation of th…

68c6013

…e job output file paths

lairdm reviewed Jul 5, 2018

View reviewed changes

Fix to fully use the os.path manipulation for dir location

8a71d26

lairdm approved these changes Jul 5, 2018

View reviewed changes

dlstadther approved these changes Jul 5, 2018

View reviewed changes

dlstadther merged commit 1192003 into spotify:master Jul 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additions to provide support for the Load Sharing Facility (LSF) job scheduler #2373

Additions to provide support for the Load Sharing Facility (LSF) job scheduler #2373

markmcdowall commented Mar 13, 2018 •

edited

Loading

dlstadther commented Mar 13, 2018

markmcdowall commented Mar 13, 2018

dlstadther commented Mar 13, 2018

Tarrasch commented Mar 13, 2018

markmcdowall commented Apr 9, 2018

dlstadther left a comment

markmcdowall commented Apr 9, 2018 •

edited

Loading

markmcdowall commented May 9, 2018

dlstadther commented May 9, 2018

lairdm Jun 13, 2018

lairdm Jun 13, 2018

markmcdowall Jul 4, 2018

lairdm Jun 13, 2018

markmcdowall Jul 4, 2018

lairdm Jun 13, 2018

lairdm Jun 13, 2018

lairdm Jun 13, 2018

lairdm Jun 13, 2018

lairdm Jun 13, 2018

markmcdowall Jul 4, 2018

markmcdowall commented Jul 4, 2018

lairdm Jul 5, 2018

markmcdowall Jul 5, 2018

lairdm commented Jul 5, 2018

dlstadther left a comment

markmcdowall commented Jul 12, 2018

Additions to provide support for the Load Sharing Facility (LSF) job scheduler #2373

Additions to provide support for the Load Sharing Facility (LSF) job scheduler #2373

Conversation

markmcdowall commented Mar 13, 2018 • edited Loading

Description

Motivation and Context

Have you tested this? If so, how?

Notes

dlstadther commented Mar 13, 2018

markmcdowall commented Mar 13, 2018

dlstadther commented Mar 13, 2018

Tarrasch commented Mar 13, 2018

markmcdowall commented Apr 9, 2018

dlstadther left a comment

Choose a reason for hiding this comment

markmcdowall commented Apr 9, 2018 • edited Loading

markmcdowall commented May 9, 2018

dlstadther commented May 9, 2018

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

Choose a reason for hiding this comment

markmcdowall commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lairdm commented Jul 5, 2018

dlstadther left a comment

Choose a reason for hiding this comment

markmcdowall commented Jul 12, 2018

markmcdowall commented Mar 13, 2018 •

edited

Loading

markmcdowall commented Apr 9, 2018 •

edited

Loading