-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Indicate current shell when misconfigured #12483
base: main
Are you sure you want to change the base?
Conversation
We require contributors to sign our Contributor License Agreement and we don't have one on file for @johnyaku. In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @johnyaku,
Thank you for submitting this pull request. I have a couple recommendations for improvement that I would like you to check out.
On top of those, another thing you should address is how this will appear for users on Windows. The process of determining the active running shell is a bit different; therefore, I don't think it's important to include. But, I would rather not have these users see empty parenthesis (e.g. ()
).
# Perhaps this link would be appropriate? https://docs.conda.io/projects/conda/en/latest/commands/init.html | ||
# Remind users what shell they are currently using. | ||
# Can be different in job schedulers versus interactive sessions, for example | ||
shell = os.popen('echo $SHELL').read().rstrip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell = os.popen('echo $SHELL').read().rstrip() | |
shell = os.getenv("SHELL") |
No need to open a sub process here. Was there a special reason you were doing that?
# Perhaps this link would be appropriate? https://docs.conda.io/projects/conda/en/latest/commands/init.html | ||
# Remind users what shell they are currently using. | ||
# Can be different in job schedulers versus interactive sessions, for example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Perhaps this link would be appropriate? https://docs.conda.io/projects/conda/en/latest/commands/init.html | |
# Remind users what shell they are currently using. | |
# Can be different in job schedulers versus interactive sessions, for example |
It's not necessary to include these extra pieces of documentation. I believe it's pretty straightforward what's going on here.
I have also updated this branch with the latest changes from main, so you will have to perform the following command to get everything working correctly locally for you:
|
Unfortunately the |
Description
On HPC systems, the shell used by the job scheduler may not always be the same as that used in interactive sessions. On our HPC, for example, the job scheduler uses
csh
while interactive sessions start inbash
. This confuses users who may have configured, say,~/.bashrc
to initialise conda but may not be aware that the job scheduler uses a different shell by default. These errors can be difficult to debug.This PR includes the path to the current shell (taken from the
$SHELL
environment variable) in the error message. Hopefully this will make it easier to spot the fact that the current shell is not the expected shell, and to adjust the job submission flags accordingly.This PR uses
os.popen()
. Not sure if this is best practice, but the spirit of the PR is to make these errors easier to debug.Checklist - did you ...
news
directory (using the template) for the next release's release notes?Don't think this change is big enough to warrant any of the above.