PR: Append paths that come from Spyder's Python path manager to the end of sys.path

[mrclary]

Update os.environ['PYTHONPATH'] along with sys.path.

PYTHONPATH paths are inserted at position 0 instead of position 1 on sys.path update because the <env>/bin path is not included (why?), consistent with expected IPython behavior.

See spyder-ide/spyder#17512

[ccordoba12]

What's your sys.path on Mac with this change? This is mine with /home/carlos/Projects/spyder added through our Pythonpath manager and I think it's fine, right?

[mrclary]

The image you show illustrates the current behavior. If you deactivate the path and reactivate it, you'll find that the position of the path moves to just under python37.zip.

With this PR, the initial position should be at the top of sys.path; deactivating and reactivating the path will put it in the same spot. Furthermore, the current behavior does not affect os.environ['PYTHONPATH'] in the IPython Console environment. With this PR, os.environ['PYTHONPATH'] is also updated; this should produce the expected behavior when users run subprocesses in the IPython Console environment.

[ccordoba12]

With this PR, the initial position should be at the top of sys.path

Why? I think that's not what we want because then the paths that come from our Python path manager would shadow standard library modules, which is really bad.

Instead, they should be at the bottom by default, i.e. below all important directories. However, if the user enables an option on the Spyder interface to add them at the top, then we should do that.

[mrclary]

With this PR, the initial position should be at the top of sys.path

Why?

Because that is the standard Python/IPython behavior. See https://docs.python.org/3/using/cmdline.html#envvar-PYTHONPATH.
In particular, the default search path(s) are "always appended to PYTHONPATH" (emphasis theirs).

I think that's not what we want because then the paths that come from our Python path manager would shadow standard library modules, which is really bad.

I think this is a risk that all Python users face and is not unique to Spyder or Spyder users. I think it is within the scope of Spyder to protect itself from users shadowing standard libraries (e.g. SPY_PYTHONPATH mechanism) but, IMHO, I think it is outside the scope of Spyder to protect users from themselves.

Instead, they should be at the bottom by default, i.e. below all important directories. However, if the user enables an option on the Spyder interface to add them at the top, then we should do that.

I did not intend to add a new feature with this PR, only to enforce consistent PYTHONPATH behavior (and consistent with Python documentation). If we introduce this option, however, python subprocesses run by the user may behave inconsistently because neither Spyder nor the user can control where PYTHONPATH paths will be placed in the sys.path of subprocesses; default search path(s) will always append PYTHONPATH.

[mrclary]

@CAM-Gerlach, you have a lot of experience with Python standards; do you have any opinion on this topic? Perhaps there is something that I'm missing.

[ccordoba12]

Because that is the standard Python/IPython behavior

Even if this is the standard behavior, I think it's a bad practice that can catch a lot of newbies off guard and without a simple way to understand how to solve problems generated by this. Using Scientific Python is hard enough as it is to add more hurdles for users.

I think it is outside the scope of Spyder to protect users from themselves.

Actually, we do a lot of things to protect them. For instance, we already prevent user modules to shadow standard library ones. Since that's our current practice, I don't agree with changing it.

Another argument against this change is that many, many users copy/paste random instructions they find on the web to solve installation issues without understanding how Python or PYTHONPATH works. Given that, we really need to prevent them from shooting themselves in the foot.

I did not intend to add a new feature with this PR, only to enforce consistent PYTHONPATH behavior

As I said, we can add an option to Spyder so that power users who want to enforce the standard behavior can do it. But I'm strongly -1 on this being the default.

[ccordoba12]

Additionally, I think this PR doesn't work as intended given the screenshot I posted above. Instead, this should be moved to Spyder and set the PYTHONPATH using a kernel handler when the console prompt is ready. That would be similar to what I did to set the initial working directory in this PR:

https://github.com/spyder-ide/spyder/pull/17760/files#diff-97f7e57fefbd2a6cb72315f662a9a1d9d1be9e66a60358cddb1983e122c6708eR355

[mrclary]

Additionally, I think this PR doesn't work as intended given the screenshot I posted above. Instead, this should be moved to Spyder and set the PYTHONPATH using a kernel handler when the console prompt is ready. That would be similar to what I did to set the initial working directory in this PR:

https://github.com/spyder-ide/spyder/pull/17760/files#diff-97f7e57fefbd2a6cb72315f662a9a1d9d1be9e66a60358cddb1983e122c6708eR355

I think I may have incorrectly cloned the spyder-kernels subrepo by mistake. That should be fixed on the latest push. sys.path and PYTHONPATH should work as advertised in IPython Console.

[mrclary]

I added the modal window as suggested on the latest push as well.

[mrclary]

I think it is outside the scope of Spyder to protect users from themselves.

Actually, we do a lot of things to protect them. For instance, we already prevent user modules to shadow standard library ones. Since that's our current practice, I don't agree with changing it.

Where do we already prevent user modules from shadowing standard library ones? I'm aware that we do that to shield IPython Console (SPY_PYTHONPATH), and we take steps to protect Jedi and the PyLS, but where do we do it to protect the users own code execution?

Another argument against this change is that many, many users copy/paste random instructions they find on the web to solve installation issues without understanding how Python or PYTHONPATH works. Given that, we really need to prevent them from shooting themselves in the foot.

I did not intend to add a new feature with this PR, only to enforce consistent PYTHONPATH behavior

As I said, we can add an option to Spyder so that power users who want to enforce the standard behavior can do it. But I'm strongly -1 on this being the default.

While I respectfully disagree that we need to prevent them from shooting themselves in the foot, I will be happy to add this feature to this PR with the default set to append PYTHONPATH to sys.path.

[mrclary]

I will be happy to add this feature to this PR with the default set to append PYTHONPATH to sys.path.

Unless it would be easier to do this in a followup PR since that will require adding a configuration setting. Let me know which you prefer.

[CAM-Gerlach]

Ryan: "I would like to propose—"
Carlos Right, Ace Developer:

The ideal solution is a checkbox in the Python path manager that controls whether the paths are inserted or extended. But if that's not immediately practical, I would recommend sticking with the standard Python behavior in this particular instance, because:

• It is the standard behavior that what advanced users comfortable enough to manipulate the Python search path expect.
• It is also required for any case where they deliberately want to override anything higher on the path (as I've occasionally had had to do for site-package-installed module for specific purposes), which would otherwise force them to either hack the stdlib/installed source code or runtime path manipulation, neither of which is always even possible much less advisable.
• If they are importing their environment's PYTHONPATH, this means their code that works when developed and tested within Spyder may suddenly break when run normally outside of it, which is likely much more potentially surprising, problematic and confusing to the experienced developers for which this feature is intended (certainly more so than consistent behavior within Spyder).

Even if this is the standard behavior, I think it's a bad practice that can catch a lot of newbies off guard and without a simple way to understand how to solve problems generated by this. Using Scientific Python is hard enough as it is to add more hurdles for users.

I certainly run into my fair share of newbies shadowing stdlib modules, in fact I just helped one a couple days ago. However, this that this is an opt-in advanced feature that's very explicitly stated to be only for power users who are comfortable with how the Python search path worked to begin (and we can add even more blaring warnings if needed), who would immediately recognize what had happened if they shadowed a stdlib module and did not intend it if anything because it is a well known beginner mistake (while one rarely if ever sees non-beginners confused by it).

If users still ignore all the warnings and use this fairly difficult to accidentally discover advanced feature anyway and ignore all the warnings, then they can still cause other trouble with it, and if we do add a checkbox, there's nothing technically stopping them from clicking it either if they've made it that far. And at least if things do still break in spite of all of this, it will be in a way consistent with the other 98% of the Python ecosystem so that any help they find will be more useful and relevant.

Another argument against this change is that many, many users copy/paste random instructions they find on the web to solve installation issues without understanding how Python or PYTHONPATH works. Given that, we really need to prevent them from shooting themselves in the foot.

Thanks to @mrclary 's spyder-ide/spyder#17512 , we already do that by not using the PYTHONPATH set outside Spyder; the only way users can shoot themself in the foot is after specifically going and finding Spyder's PYTHONPATH manager, figuring out how to use it, ignoring the warnings and then still proceeding to shoot themselves in the foot with it. And fortunately, I've seen a lot less of those instructions that involve PYTHONPATH manipulation nowadays. At the end of the day, the only firearm lock that kids can't bypass if they are really determined to do so is one that didn't have a way for adults to access the firearm when they really need it.

Where do we already prevent user modules from shadowing standard library ones? I'm aware that we do that to shield IPython Console (SPY_PYTHONPATH), and we take steps to protect Jedi and the PyLS, but where do we do it to protect the users own code execution?

The current directory/project path is appended to the path, not injected at sys.path[0] (as when running Python normally), as you can see in @ccordoba12 's sys.path above. The latter is considered to be a serious security and usability flaw, and we (well, mostly Victor, I just helped with the docs) just added a new option -P to Python 3.11 to disable that legacy behavior, in addition to -I which already does so, with the intention of eventually making it the default. Therefore, if you create a file, e.g. string.py, in the CWD/project directory, the stdlib string will still get be imported.

[ccordoba12]

I think I may have incorrectly cloned the spyder-kernels subrepo by mistake. That should be fixed on the latest push.

@mrclary, the problem is that there are two different behaviors now (I checked again with your latest changes):

1. When Spyder creates a new IPython console, paths in our Python Path Manager (PPM) are added to the bottom of sys.path (my screenshot).
2. When you go to PPM and add new paths, they will be added to the top (what you said).

This is an UX inconsistency that we need to solve so that things are uniform (i.e. paths are always added to the top or bottom, regardless of the situation). That's why I suggested to use a kernel handler (now I see we have one, called update_syspath) to update sys.path with the PPM paths after the console is created but before users can enter a command on it. In other words, when the prompt is ready. If this is not clear for you, I could explain you how to do it in a short Zoom call.

By the way, that would make useless the code you changed here in spydercustomize.py, so you could remove it.

I'm aware that we do that to shield IPython Console (SPY_PYTHONPATH), and we take steps to protect Jedi and the PyLS, but where do we do it to protect the users own code execution?

I was referring to that, sorry for the misunderstanding.

While I respectfully disagree that we need to prevent them from shooting themselves in the foot, I will be happy to add this feature to this PR with the default set to append PYTHONPATH to sys.path.

Ok, thanks for understanding. Then, please work on your Spyder PR to add what @CAM-Gerlach said:

The ideal solution is a checkbox in the Python path manager that controls whether the paths are inserted or extended

with that checkbox being disabled by default.

It is the standard behavior that what advanced users comfortable enough to manipulate the Python search path expect.

The problem is that Spyder makes way too easy to alter PYTHONPATH (on Windows is even worse because users can export it to their env vars, so that would affect all their other Python envs). So what's normally a feature for power users, it's now within reach of any newbie programmer.

That's why I think we need to be really conservative about this change.

[mrclary]

I think I may have incorrectly cloned the spyder-kernels subrepo by mistake. That should be fixed on the latest push.

@mrclary, the problem is that there are two different behaviors now (I checked again with your latest changes):

1. When Spyder creates a new IPython console, paths in our Python Path Manager (PPM) are added to the bottom of sys.path (my screenshot).
2. When you go to PPM and add new paths, they will be added to the top (what you said).

This is an UX inconsistency that we need to solve so that things are uniform (i.e. paths are always added to the top or bottom, regardless of the situation). That's why I suggested to use a kernel handler (now I see we have one, called update_syspath) to update sys.path with the PPM paths after the console is created but before users can enter a command on it. In other words, when the prompt is ready. If this is not clear for you, I could explain you how to do it in a short Zoom call.

@ccordoba12 I am unable to reproduce this behavior. For me, new IPython Consoles at Spyder launch as well as new Console tabs place the PPM paths at the top of sys.path. I checked this for "Same as Spyder" and external environments (with the current developer spyder-kernels installed).

[CAM-Gerlach]

Let me know if you want/need me to test something on Windows...

@ccordoba12 I am unable to reproduce this behavior. For me, new IPython Consoles at Spyder launch as well as new Console tabs place the PPM paths at the top of sys.path. I checked this for "Same as Spyder" and external environments (with the current developer spyder-kernels installed).

Just to be clear, did you open a new console, then go into the PYTHONPATH manager and add a new path, then click OK, then check the console again? Or only test with the existing PYTHONPATH manager paths without changing them after the console was started (or are they only intended to take effect on console start)?

The problem is that Spyder makes way too easy to alter PYTHONPATH. So what's normally a feature for power users, it's now within reach of any newbie programmer.

I mean, what said above that I was responding to was that it is way too easy for newbie programmers to copy code they found online to set env variables, modify PYTHONPATH and shadow things, which @mrclary 's changes prevents. Now you're saying the Spyder makes it too easy to set the PYTHONPATH internally, but like I said, if that's a problem we can add a red-letter warning in the dialog text, and/or a modal warning dialog they have to click through when they click the "insert at the front of path" checkbox.

On Windows is even worse because users can export it to their env vars, so that would affect all their other Python envs

Yes, but this only affect running Python outside of Spyder, where it makes no difference where Spyder inserts the path internally, and furthermore it means that things would suddenly break once they try to run their code outside of Spyder as opposed to immediately after making the change and consistently between Spyder and elsewhere, which is potentially much more difficult for users to figure out and debug.

[mrclary]

Let me know if you want/need me to test something on Windows...

@ccordoba12 I am unable to reproduce this behavior. For me, new IPython Consoles at Spyder launch as well as new Console tabs place the PPM paths at the top of sys.path. I checked this for "Same as Spyder" and external environments (with the current developer spyder-kernels installed).

Just to be clear, did you open a new console, then go into the PYTHONPATH manager and add a new path, then click OK, then check the console again? Or only test with the existing PYTHONPATH manager paths without changing them after the console was started (or are they only intended to take effect on console start)?

Yes. See below screen capture. The initial startup console behaves correctly as well as new consoles, in multiple environments, and after updating in PPM. So I'm still not sure what circumstances produced @ccordoba12's screenshot.

I mean, what said above that I was responding to was that it is way too easy for newbie programmers to copy code they found online to set env variables, modify PYTHONPATH and shadow things, which @mrclary 's changes prevents. Now you're saying the Spyder makes it too easy to set the PYTHONPATH internally, but like I said, if that's a problem we can add a red-letter warning in the dialog text, and/or a modal warning dialog they have to click through when they click the "insert at the front of path" checkbox.

I'm not sure, exactly, what my changes prevent that you're referring to here. I don't think it is a problem to make it "too easy" to change PYTHONPATH (PPM). These are just extra search paths; even newbies may need to add a local directory to their Python search path. Regarding "red-letter warnings", while it is useful to be informative to users, I think this would inflate the threat.

On Windows is even worse because users can export it to their env vars, so that would affect all their other Python envs

Yes, but this only affect running Python outside of Spyder, where it makes no difference where Spyder inserts the path internally, and furthermore it means that things would suddenly break once they try to run their code outside of Spyder as opposed to immediately after making the change and consistently between Spyder and elsewhere, which is potentially much more difficult for users to figure out and debug.

Yes, I think this is just the risk of treating extra search paths in Spyder differently than the standard Python/IPython protocol. With the added feature of #18308, we should have clear messaging about the affects of prepending/appending PYTHONPATH to sys.path.

[CAM-Gerlach]

So I'm still not sure what circumstances produced @ccordoba12's screenshot.

OS-specific differences? Did you try on Binder?

These are just extra search paths; even newbies may need to add a local directory to their Python search path.

Well, they shouldn't, at least if they're new; Spyder automatically adds the current working directory (project dir, etc) so if they are working directly in that, their own packages/modules will work fine. If they have other Python library code they need to access, they should just install them properly rather than doing manual path hacking, unless they are an experienced developer (like you) who fully understand what they are doing and have a specific need to do it.

Yes, I think this is just the risk of treating extra search paths in Spyder differently than the standard Python/IPython protocol. With the added feature of #18308, we should have clear messaging about the affects of prepending/appending PYTHONPATH to sys.path.

Yes, indeed, at least to the extent we're exposing the equivalent of standard Python features and calling them such (this doesn't necessarily extend as far as replicating Python's discouraged sys.path[0] behavior with certain options, or Spyder's own automatic appending of the user's project/etc. directory to sys.path).

[mrclary]

Well, they shouldn't, at least if they're new; Spyder automatically adds the current working directory (project dir, etc) so if they are working directly in that, their own packages/modules will work fine.

My company employs 50+ physicists, many of which use Spyder, and many of which I consider "newbies". We have a set of proprietary Python tools that we distribute via git. So yes, "newbies" should have extra search paths. For those that don't use Spyder, we have to advise them to modify their system PYTHONPATH. For those that do use Spyder, the PPM is thankfully much easier. Perhaps someday I'll create whls for these Python tools and they can just install them and not worry about PYTHONPATH, but that is far down the priority list.

[mrclary]

OS-specific differences? Did you try on Binder?

How do I do that for a specific pull request?

[CAM-Gerlach]

/show binder

[mrclary]

/show binder

I don't know what that means...

[CAM-Gerlach]

That was supposed to trigger our GHA bot to post a comment with the generated Binder link to launch it on this PR, but it seems it didn't work. @ccordoba12 do you know what's going on? It seems the binder chat workflow never triggered even though it looks right in the workflow YML.

[mrclary]

That was supposed to trigger our GHA bot to post a comment with the generated Binder link to launch it on this PR, but it seems it didn't work. @ccordoba12 do you know what's going on? It seems the binder chat workflow never triggered even though it looks right in the workflow YML.

Maybe you meant to do that in the spyder repo instead of here in the spyder-kernels repo?

[CAM-Gerlach]

🤦

[mrclary]

I still cannot reproduce @ccordoba12's screen shot. I've tried on my Ubuntu VM and still get the expected behavior with this PR (in conjunction with #17512).

[ccordoba12]

I tested this locally and it's working as expected. Thanks @mrclary!