search

lanl / BEE

Other
13 stars 3 forks source link

Slurm Worker Should Respect beeflow:ScriptRequirement $SHELL Option #846

Closed rstyd closed 1 week ago

rstyd commented 1 month ago

This issue is a follow on for #845. Once we have added a $SHELL configuration option to the beeflow:ScriptRequirement. We need to ensure that the BEE worker respects that. Most of the current sbatch commands are just slurm commands so those will work fine regardless which shell interpreter the user is using. There are a few points where we'll have to adjust.

The slurm_worker currently hard codes the $SHELL on line 67. We should convert this to using whichever $SHELL is defined in the BEE config.
https://github.com/lanl/BEE/blob/c3b2d406e9e148bde634a36a9ccfff898540e1f9/beeflow/common/worker/slurm_worker.py#L66-L74

The flux_worker will also need to be changed. https://github.com/lanl/BEE/blob/c3b2d406e9e148bde634a36a9ccfff898540e1f9/beeflow/common/worker/flux_worker.py#L46-L50

The only area that I think could be an issue is where we use the set builtin for slurm and flux.

https://github.com/lanl/BEE/blob/c3b2d406e9e148bde634a36a9ccfff898540e1f9/beeflow/common/worker/slurm_worker.py#L82-L83

https://github.com/lanl/BEE/blob/c3b2d406e9e148bde634a36a9ccfff898540e1f9/beeflow/common/worker/flux_worker.py#L47-L48

I'm not sure off the top of my head but this might not behave similarly depending on which shell the user prefers.

kchilleri commented 1 week ago

PR#856 covers this issue for the Slurm worker. I will work on adding this to the Flux worker.

pagrubel commented 1 week ago

PR#856 covers this issue for the Slurm worker. I will work on adding this to the Flux worker.

@kchilleri That would be great, would you please open a separate issue for flux and we can close this one.