ec-jrc / pyPoseidon

Framework for Hydrodynamic simulations
https://pyposeidon.readthedocs.io/
European Union Public License 1.2
20 stars 8 forks source link

Manage mpirun existence #121

Closed brey closed 1 year ago

brey commented 1 year ago

https://github.com/ec-jrc/pyPoseidon/blob/a3bff4bc890191b38bf8c255607deb06d17b7e5f/pyposeidon/tools.py#L97

The setup above fails ungracefully when mpirun doesn't exist. Switching to something like

        with subprocess.Popen(
            shlex.split(cmd),
            cwd=calc_dir,
            shell=True,
            stderr=subprocess.PIPE,
            stdout=subprocess.PIPE,
            universal_newlines=True,
            # bufsize=1,
        ) as ex:
            output = ex.stdout.read()
            error = ex.stderr.read()

       if error:
             gracefull exit...

should do the trick.

pmav99 commented 1 year ago

Generally speaking there are two ways to handle these things:

  1. EAFP = Easier to Ask for Forgiveness than Permission
  2. LBYL = Look Before You Leap

A more in depth explanation can be found here.

The main downside with LBYL is race conditions. E.g. you check that a file exists and then you open/call/execute it. In the meantime the file might have been removed/renamed/overwriiten/replaced/whatever. So when you execute it it can be a completely different file than the one you checked that it was existing.

Therefore, when it is about file manipulation (create a file, open a file etc), then usually we use EAFP

In this case, though, I think i would use LBYL. The existence of mpirun is a prerequisite of solving the model. Therefore the fact that mpirun is not installed, should probably be checked at the beginning of the procedure. So I would define something like this (not tested):

import shutil

def is_mpirun_installed() -> bool:
    return bool(shutil.which("mpirun"))

def is_schism_installed() -> bool:
    return bool(shutil.which("schism"))

def check_schism_prerequisites() -> None:
    checks = dict(
        mpirun=is_mpirun_installed(),
        schism=is_schism_installed(),
    )
    missing = [key for key, is_installed in checks.items() if not is_installed]
    if missing:
        raise ValueError(f"The following executables are missing. Please install them and try again: {missing}")

# ....
# And somewhere early in the schism module:
check_schism_prerequisites()

This way we would get an early stop and a helpful Error message.