Ticket #250 (closed enhancement: fixed)

Opened 12 years ago

Last modified 7 years ago

Please replace dps run.sh with a callable function

Reported by: rowen Owned by: daues
Priority: normal Milestone:
Component: pex_harness Keywords:
Cc: RayPlante Blocked By:
Blocking: Project: LSST
Version Number: 3383
How to repeat:

not applicable

Description

It would be nice if run.sh could be replaced with a callable function. That would avoid the need to hard-code information and to have a private copy of a function that might be updated in the future.

It could easily be replaced by a python script that used subprocess.call to execute the various things. If you do use subprocess that then I suggest supplying the arguments as a list of strings (rather than the whole command as one string) so that no shell has to be invoked, e.g.:

subprocess.call("mpdboot", "--totalnum=%s" % (nodes,)", "--file=nodelist.scr", "--verbose")

I'd be happy to provide a python equivalent to run.sh if desired.

Attachments

startPipeline.py (3.7 KB) - added by rowen 12 years ago.
Python function to replace run.sh; copy of official version in imageproc pipeline/examples

Change History

comment:1 Changed 12 years ago by rowen

I have attached a python module startPipeline.py that replaces run.sh. Advantages include:

  • All pipelines can use the same function instead of each needing a private copy. Thus it can be updated in one location and all pipelines benefit.
  • It computes nnodes and nslices from the node list file.

I have tested it on the example pipelines in imageproc and they all work. In fact it is checked into imageproc in pipeline/examples and all the example pipelines use it. If/when it shows up in dps I'll delete the version in imageproc and modify the example pipelines to use the official version.

comment:2 Changed 12 years ago by daues

Accept ticket; will work on merging the startPython script into dps.

comment:3 Changed 12 years ago by daues

  • Status changed from new to assigned

Forgot to click accept.

comment:4 Changed 12 years ago by rowen

I see that the machine file format is more complicated than I thought -- in particular a host can be specified without a number of slices after it. I suspect that the number of slices cannot be determined from the machine file (though the number of nodes can). If that's so, the number of slices should probably be an input argument.

comment:5 Changed 12 years ago by rowen

OK, I was wrong. If a node has no :slices then it slices is 1. I've updated the copy on imageproc trunk and will update the copy here as well.

Changed 12 years ago by rowen

Python function to replace run.sh; copy of official version in imageproc pipeline/examples

comment:6 Changed 10 years ago by daues

  • Status changed from assigned to inTicketWork

comment:7 Changed 10 years ago by daues

  • Cc RayPlante added
  • Status changed from inTicketWork to inStandardsReview
  • Component changed from dps to pex_harness
  • reviewstatus changed from notReady to needsReview
  • Owner changed from daues to rowen

In the "production" DC3a runs the orchestration layer ctrl_orca utilizes the script $PEX_HARNESS_DIR/bin/launchPipeline.py with appropriate command line arguments to start each pipeline. This script makes use of a callable function

def launchPipeline(policyFile, runid, name=None, verbosity=None, nodesfile="nodelist.scr"):
    """
    Launch a pipeline with a given policy and Run ID.  A name, verbosity level, and
    node list file may also be provided. If no node list is provided, a file named "nodelist.scr"
    in the current directory will be used.  If the policy_file refers to other policy files,
    the path to those files will taken to be relative to the current directory.
    """

to initiate the execution. Within the scope of this ticket we (1) move this callable function from bin/launchPipeline.py to python/lsst/pex/harness/run.py to make this function more readily available, and (2) switch the pex_harness examples to use launchPipeline.py. Settling on the single job submission script "launchPipeline.py", we remove the run.sh files from examples/*, and change the documentation (examples/*/README's , doc/*.docx) to utilize launchPipeline.py.

comment:8 Changed 10 years ago by rowen

One comment on launchPipeline.py: it looks very useful, but would be even more so if the subroutine launchPipeline (and any support subroutines -- what is getNode used for?) was in lsst.pex.harness instead of being hidden in a command-line script.

That way one can easily write a command-line script that takes user arguments that describe which data to process and perhaps some settings for how to process it and launches a pipeline accordingly. This is something I did for difference imaging in DC2 and found very useful.

Also, please consider rewriting launchPipeline to use the subprocess module instead of os.execvp. Admittedly subprocess.Popen probably calls os.execvp under the hood, but subprocess is now the recommended way to handle subprocesses and the like; the older calls are not necessarily well maintained and are less cross-platform compatible.

comment:9 Changed 10 years ago by rowen

  • Owner changed from rowen to RayPlante

Sorry about that last comment -- I hadn't realized the new code was on ticket 250.

Here is my review of ticket 250. Note: I'd be happy to implement or help implement any of these suggested changes):

file bin/launchPipeline.py

  • please delete function getNode; it is not used
  • please specify the help strings for optparse options in add_option(...) rather than in the usage string; this makes the help much more likely to stay correct as the code evolves
  • I suggest moving the code from usage = down through logger = createLog() into main(). A second choice would be to ditch main. I don't see the logic as to which lines are included and which are excluded.
  • Please replace
        except SystemExit:
            pass
        except:
    
    with
        except Exception:
    
    There is no longer any need to catch and ignore or reraise SystemExit and the like due to the improved exception hierarchy in Python 2.4 and later
  • several imports aren't used, including shutil and subprocess; please weed out the unused ones
  • the with statement isn't used so from future import with_statement isn't needed (though as a big believer in the with statement, I could see leaving it in just in case it's wanted later)

files bin/launchPipeline.sh, runPipeline.py, runPipeline.sh

I suspect these are all unnecessary. The first two are probably redundant with no work (other than looking for examples and tests that use them).

The current subroutine launchPipeline calls runPipeline.sh but I think the code would be much cleaner if instead of doing so, it used implemented the logic itself using the subprocess module (inside the function launchPipeline or as a new function). The advantages are:

  • No need to pass data as command-line arguments; it can be directly used as variables
  • No need to launch another subprocess
  • The code will be more flexible and probably shorter
  • The code will be in one central place

python/.../run.py

  • Would it make sense to hoist the contents of run.py into lsst.pex.harness (by putting "from run.py import *" in init.py)? If run.py contains private symbols then you can easily hide them by making their name start with _ and/or you can specify a list of public symbols all = ["launchPipeline", ...].
  • consider simplifying this code:
    if node.find(':') >= 0:
      (node, n) == node.split(':')
    else:
      n = ''
    
    by using partition (new to Python 2.5):
    (node, sep, n) = node.partition(":")
    
  • please use the subprocess module instead of os.execvp; it is more cross-platform and future-proof. If you do that, then supply the arguments as a list, instead of a formatted string; this is faster and avoids starting an unnecessary shell.
  • I'm skeptical of this code: cmd = "runPipelin.sh.py %s..." how can this work? I would not expect there to be a command named "runPipelin.sh.py" but perhaps the first argument is ignored -- if so, this issue will go away if you switch to subprocess -- especially if you replace runPipeline.foo with direct subprocess calls in the launchPipeline subroutine itself
  • Is createLog useful outside of the launchPipeline subroutine? A global search showed no uses in pex_harness itself. If you don't see it as useful to outsiders then I would ditch it and move the one line of code into the launchPipeline function itself.
  • Why are addAllVerbosityOptions and addVerbosityOption inside of run.py when they are only used by launchPipeline.py? If you see them as potentially reusable then that's a good answer; otherwise I'd move them to launchPipeline.py -- where they are used.

Finally, please delete python/.../startPipeline.py, which is what I wrote for DC2 that launchPipeline.py is replacing.

comment:10 Changed 10 years ago by rowen

  • Owner changed from RayPlante to dgehrig
  • Status changed from inStandardsReview to inTicketWork

comment:11 Changed 10 years ago by ktl

  • Status changed from inTicketWork to assigned
  • Owner changed from dgehrig to daues

comment:12 Changed 10 years ago by robyn

Hello Greg,

This request is 22 months old. Is this ticket still viable? If not, please close it.

comment:13 Changed 10 years ago by daues

  • Status changed from assigned to closed
  • Resolution set to fixed

Closing as ticket 250 was merged to trunk.

comment:14 Changed 7 years ago by robyn

  • Milestone DC3 Design deleted

Milestone DC3 Design deleted

Note: See TracTickets for help on using tickets.