AgnostiqHQ / covalent-slurm-plugin

Executor plugin interfacing Covalent with Slurm
https://covalent.xyz
Apache License 2.0
27 stars 6 forks source link

Allow for plugin use without Conda and enable a new `bashrc_path` config variable #61

Closed Andrew-S-Rosen closed 1 year ago

Andrew-S-Rosen commented 1 year ago

Closes #60.

### Added

- A new config variable, `bashrc_path`, which is the path to the bashrc script to source.

### Changed

- Removed automatic sourcing of `$HOME/.bashrc` from the SLURM submit script.

### Fixed

- Does not put conda-related lines in SLURM script if `conda_env` is set to `False` or `""`..
- Changed default config value of `conda_env` from `None` to `"base"`, but it has the same effect as before.
- A proper `ValueError` will now be raised if `ssh_key_file` is not supplied.
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.17 :tada:

Comparison is base (82f507a) 84.59% compared to head (92d2b27) 85.76%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #61 +/- ## =========================================== + Coverage 84.59% 85.76% +1.17% =========================================== Files 2 2 Lines 305 309 +4 =========================================== + Hits 258 265 +7 + Misses 47 44 -3 ``` | [Impacted Files](https://app.codecov.io/gh/AgnostiqHQ/covalent-slurm-plugin/pull/61?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ) | Coverage Δ | | |---|---|---| | [covalent\_slurm\_plugin/slurm.py](https://app.codecov.io/gh/AgnostiqHQ/covalent-slurm-plugin/pull/61?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ#diff-Y292YWxlbnRfc2x1cm1fcGx1Z2luL3NsdXJtLnB5) | `85.71% <100.00%> (+1.17%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Andrew-S-Rosen commented 1 year ago

@wjcunningham7 --- thoughts on this one? I think this should be ready to go whenever you agree.

Andrew-S-Rosen commented 1 year ago

One comment:

Andrew-S-Rosen commented 1 year ago

EDIT: Fixed and ready to go!!

@wjcunningham7 --- FYI there are some sporadic GitHub actions test failing both PRs. Might need to have them retriggered later.

wjcunningham7 commented 1 year ago

One comment:

  • My personal opinion is that we should remove the source $HOME/.bashrc line. We don't want to force the sourcing of the .bashrc (for instance, on some machines, there are several variants of .bashrc files and the usual $HOME/.bashrc isn't always the desired one). I've removed this hard-coded line and added a bashrc_path config variable.

I would prefer to find a way to avoid sourcing bashrc altogether (for conda) since all sorts of unrelated stuff can be in there, and if the default shell isn't bash it will fail entirely. This may be out of scope for this PR however. For the scope of this PR, your changes are appropriate.

Andrew-S-Rosen commented 1 year ago

It wasn't actually clear to me why we are sourcing the bashrc. Wouldn't the bashrc be sourced already, just like it is at login? I don't think I've ever had to source the bashrc in my Slurm jobs, personally.

wjcunningham7 commented 1 year ago

It wasn't actually clear to me why we are sourcing the bashrc. Wouldn't the bashrc be sourced already, just like it is at login? I don't think I've ever had to source the bashrc in my Slurm jobs, personally.

It depends on a few things -- the default shell on the system, the behavior of sshd, and the behavior of asyncssh. For perlmutter it looks like this may not be necessary. In general, the bashrc file is only sourced if the shell is interactive. Additionally, even if SSHD does source bashrc for non-interactive connections, the bashrc skeleton (at /etc/skel/.bashrc) often includes a return statement for non-interactive shells. Since there are a few variables in play here, we can't necessarily assume a system we haven't interacted with before will source the file automatically.

All that being said, the pattern here has been applied in a few different ways across different plugins, and it appears to me that asyncssh always uses an interactive shell. So it may not be necessary at all!

Andrew-S-Rosen commented 1 year ago

Ah that's useful context! Well, good news is that if a user doesn't want to source anything, they don't have to now. They can just do bashrc_path = False, so at worst we just have an additional config option that can maybe be deprecated eventually (and replaced with an additional prerun_command by the user if desired).