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

Enable login mode for terminal - to automatically source the /etc/profile script #4112

Merged
merged 1 commit into from
Feb 25, 2019

Conversation

dmikushin
Copy link
Contributor

Jupyter's terminal does not source in a vast majority of environment variables defined in /etc/profile and /etc/profile.d. According to sh & bash docs, these locations are sourced by "login" or "interactive" terminals. Being technically an interactive terminal, Jupyter's terminado should use interactive mode by default.

Consider for example CUDA toolkit, which is usually added into PATH using /etc/profile.d script. Without sourcing it, Jupyter terminal would confuse users not being able to find CUDA compiler.

@Carreau
Copy link
Member

Carreau commented Oct 21, 2018

That looks reasonable to me.

@kevin-bates
Copy link
Member

This change appears to have introduced a fairly significant (IMO) regression issue for multi-env python configurations in that the active env from within a launched terminal will vary from the notebook server's env for folks that have multiple envs.

This will result in issues where users would like to perform some actions relative to their notebook's environment, but find that changes performed via a terminal session are not applied (due to the difference in environments). I understand this issue is in direct conflict with the purpose of this PR, but I think this warrants some discussion.

Issues that have been created since the merge/release of this PR include #4877 and #5247 - besides the fact that I started looking into this because I had a user encountering this very issue.

@dmikushin, @Carreau, @minrk (or anyone else) - I'd like to hear your opinions and how we might alleviate this.

  • Should this be something addressed in docs?
  • Should this be something made configurable?
  • Should this change be backed out?

My feeling is that the original behavior should be the default - with a login shell behavior offered as an option (to non-Windows users).

@dmikushin
Copy link
Contributor Author

Hi @kevin-bates My logic is to make Jupyter terminal to behave as a normal Linux terminal. When you open a new terminal window in Linux (or actually in Windows as well), the default environment profile is loaded from the user settings. And - yes, whenever I change a variable locally wrt that specific terminal, it does not affect any other terminal settings.

In terminals, multiple envs are usually configured by sourcing additional scripts. For instance, on supercomputers we often need to switch environment from one compiler version to another. But this still co-exists well with the fact that we do have some default compiler brought by "-l", not just nothing. But I'm not sure if this answers your question, please let me know.

@kevin-bates
Copy link
Member

Thank you for your response @dmikushin. I understand the change and, for the most part, think it's good. But in contrast with existing behavior, it does present a regression issue that I don't think was caught during the change's introduction.

In hindsight, this should have been an opt-in behavior since it breaks expected functionality - that being that the environment is consistent with a) what notebook was started within, and b) what kernels are running within.

@dmikushin
Copy link
Contributor Author

@kevin-bates Why do you think that the state-of-art behavior (as explained above) should be opt-in, and counter-logical behavior should stay the default simply because it was historically first introduced in Jupyter?

Please kindly correct me if I'm wrong. Is it correct that your are asking that every new Jupyter terminal should inherit the state of the previous ones? That does not make sense to me and eventually leads to garbage state.

The worst thing about removing "-l" is: everything that is not in the /usr/bin is ignored, such as NVIDIA CUDA compiler, editor profiles, shell configs, etc etc. The default terminal becomes just worthless. This implies that Jupyter has to be ugly by default, which it not what I want.

@kevin-bates
Copy link
Member

The worst thing about removing "-l" is: everything that is not in the /usr/bin is ignored, such as NVIDIA CUDA compiler, editor profiles, shell configs, etc etc. The default terminal becomes just worthless.

Hmm - yes, I would agree that only having PATH=/usr/bin would be worthless, but that's not what I see. If I go back to Notebook 5.7.8, my PATH prior to launching Notebook is:

PATH=/opt/anaconda3/envs/clean/bin:/opt/anaconda3/condabin:/usr/local/opt/scala@2.11/bin:/Users/kbates/bin:/opt/anaconda3/bin:/usr/local/mongodb/bin:/opt/spark-2.4.3-bin-hadoop2.7/bin:/usr/local/opt/scala@2.11/bin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Library/TeX/texbin:/usr/local/MacGPG2/bin:/Library/Apple/usr/bin:/bin

And from within a launched terminal, my PATH is:

PATH=/opt/anaconda3/envs/clean/bin:/opt/anaconda3/condabin:/usr/local/opt/scala@2.11/bin:/Users/kbates/bin:/opt/anaconda3/bin:/usr/local/mongodb/bin:/opt/spark-2.4.3-bin-hadoop2.7/bin:/usr/local/opt/scala@2.11/bin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Library/TeX/texbin:/usr/local/MacGPG2/bin:/Library/Apple/usr/bin:/bin

In fact, the two environments are nearly identical and I don't find it useless at all. I like the fact that I have access to the same commands I had access to prior to launching notebook.

Is it correct that your are asking that every new Jupyter terminal should inherit the state of the previous ones?

My take is that the typical use case for using a terminal within a notebook environment is that it enables a mechanism that the user (data scientist, analyst, whomever) can operate outside of their notebook but relative to their notebook's environment. The use of the login shell breaks that for users that use multiple environments - which most data scientists do. So, to your question, I'm saying that every new Jupyter terminal should inherit the state of the environment in which the notebook server was launched.

Since notebook has tens of thousands of users, we (maintainers) do indeed give precedence to current behaviors and strive to minimize situations that introduce breaking changes (i.e., regressions) at the expense of new behaviors.

The problem here is that a user within a terminal session now first has to realize they're not in the correct env, then try to determine which environment they must activate in order to do their work. I'm not aware of how to do that other than remembering the env.

I'm sorry I raised what I felt was an important issue. We have a regression here, but given this was introduced on a major release boundary, there is some wiggle-room in that many would claim breaking changes can occur across such boundaries.

@minrk
Copy link
Member

minrk commented Jun 26, 2020

I think an important distinction in what the logical default is depends on the context in which a notebook is launched. I think in most non-jupyterhub cases, it is launched from an existing user shell that has already been initialized with all the login shell profile-loading, etc. In these cases, invoking the login shell from an existing login shell is at best redundant, and at worst reverting env customization that might have happened after initializing the shell, which is the issue @kevin-bates is bringing. Please correct me if I'm wrong about that on either side.

The alternative scenario, when the notebook is launched in a more "raw" environment certainly comes up a lot in JupyterHub, or in "app launchers" that might start the notebook directly via systemd/supervisor/init/app launcher/etc. without going through a login shell first. This is the case where the terminal env is weird and empty, and a login shell is ~always preferable. Another solution to these cases is to use a login shell as a wrapper for launching the notebook. This is actually what I prefer to do in JupyterHub deployments, since it ensures consistent behavior for not just the terminals, but also notebooks if there is any relevant env there.

With that in mind, I'd support making terminals starting the login shell an opt-in behavior instead of opt-out. The problem we have right now is that it isn't even opt-out—it's unconditional (for non-Windows), so it can't be overridden. At the very least, we should make it opt-out, but I think opt-in is more often the right choice. If there's a reasonable way to detect if no login shell has been invoked (hard to be rigorous since things like shopt -q login_shell are shell-dependent and aren't inherited by subprocesses), then we could make the default to load a login shell in those cases. But I don't know a good way to check for that better than a suspiciously empty $PATH.

@kevin-bates
Copy link
Member

@minrk - thank you for your response. I believe it sufficiently reflects my concern here.

I appreciate you mentioning Hub and launcher applications and I suspect that must be the kind of environment @dmikushin is running within (Dmitry, could you confirm?). A solution proposed by @dlukes in #5247 (comment) is to use the SHLVL env to determine if a login shell should be used. Since I don't use Hub or Launcher apps, it didn't dawn on me how SHLVL could ever be < 1 - which is what triggers the addition of -l in this case, but now I realize that Hub/Launcher scenarios introduce that case exactly. That strikes me as a better way to go here as it enacts the login shell when it is most needed.

@dmikushin
Copy link
Contributor Author

dmikushin commented Jun 26, 2020

The simplest amateur usecase is to launch an instance of jupyter-notebook from within the desktop session, just as a desktop application. In this case, login shell is already present, as the desktop user has to log in. On the other hand, cloud computing service deployment implies there is no user session whatsoever; Jupyter is coming as a HTTPS service, and its backends are running as daemons (init.d, systemd, etc). Here the login shell switch is essential, as it does not exist by any other means.

@kevin-bates
Copy link
Member

Yes - I understand. So it sounds like your use case is purely relative to these kinds of Hub/Launcher/Cloud services environments.

Are you in a position to try the simple addition referenced in the previously-linked comment that checks SHLVL? I think that may be a great way forward here.

@Zsailer
Copy link
Member

Zsailer commented Jun 26, 2020

I agree with @minrk here. First, at the very least, this should be configurable. The answer to whether it should be opt-in vs. opt-out question is a bit less clear in my mind. We've now introduced a "breaking-change" either way we go (since we have major release with each state). Choosing a default will break someone's setup. That said, I'd lean towards making this opt-in.

Leveraging SHLVL to find a source shell is a clever fix. It makes me a little bit nervous to implicitly set a use_login_shell trait, since you could get different behavior when testing terminals locally vs. deploying a system with terminals created from nothing. But in the strict case where SHLVL==0, it seems like a login shell is the appropriate default.

@kevin-bates kevin-bates mentioned this pull request Jun 30, 2020
24 tasks
@kevin-bates
Copy link
Member

But in the strict case where SHLVL==0, it seems like a login shell is the appropriate default.

I agree. It's also a great compromise.

Now that we know how the two experiences (retaining current env or having nothing in env) come about I'd like to see this addressed by 6.1 GA and have added it to #5423.

@dlukes - are you able to provide a PR per your previous #5247 (comment)? If we haven't heard back in a few days, I'll go ahead and add the PR with you as a co-author (assuming that will be okay).

@dlukes
Copy link
Contributor

dlukes commented Jul 1, 2020

Sure, I’d love to :) Thanks for picking up on this!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants