Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 53 commits into from
May 12, 2023
Merged

Conversation

Andrew-S-Rosen
Copy link
Contributor

@Andrew-S-Rosen Andrew-S-Rosen commented May 9, 2023

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.
  • I have added the tests to cover my changes.
  • I have updated the documentation, VERSION, and CHANGELOG accordingly.
  • I have read the CONTRIBUTING document.

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

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

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

Additional details and impacted files
@@             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 Coverage Δ
covalent_slurm_plugin/slurm.py 85.71% <100.00%> (+1.17%) ⬆️

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

@Andrew-S-Rosen
Copy link
Contributor Author

Andrew-S-Rosen commented May 9, 2023

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

covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Contributor Author

Andrew-S-Rosen commented May 10, 2023

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.

@Andrew-S-Rosen Andrew-S-Rosen changed the title Allow for plugin use without Conda Allow for plugin use without Conda and enable a new bashrc_path config variable May 10, 2023
Update conda_env type
@Andrew-S-Rosen
Copy link
Contributor Author

Andrew-S-Rosen commented May 10, 2023

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.

covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
covalent_slurm_plugin/slurm.py Outdated Show resolved Hide resolved
@wjcunningham7
Copy link
Member

wjcunningham7 commented May 12, 2023

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Andrew-S-Rosen commented May 12, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SLURM job crashes if Conda is not installed
2 participants