Skip to content

PR: Reduce calls to get environment variables in SpyderKernelSpec and raise timeout threshold to do that#23278

Merged
ccordoba12 merged 5 commits into
spyder-ide:masterfrom
mrclary:env-var-timeout
Mar 20, 2025
Merged

PR: Reduce calls to get environment variables in SpyderKernelSpec and raise timeout threshold to do that#23278
ccordoba12 merged 5 commits into
spyder-ide:masterfrom
mrclary:env-var-timeout

Conversation

@mrclary

@mrclary mrclary commented Dec 14, 2024

Copy link
Copy Markdown
Contributor

Description of Changes

I propose raising the timeout to 5s for all scenarios.
This PR raises the timeout for the subprocess for getting user environment variables from 0.5s to 5s, and reduces superfulous calls to get_user_environment_variables by moving it to the instantiation of SpyderKernelSpec from SpyderKernelSpec.env.

I've run into a scenario where get_user_environment_variables can take up to ~1s or more. This triggers the timeout and renders the function useless, leaving users with incorrect environment variables sent to the console and language server.

Currently, get_user_environment_variables is called in SpyderKernelSpec.env, which is not appropriate since this is decorated with @property. SpyderKernelSpec.env can be referenced many times, resulting in superfluous exectution of get_user_environment_variables (8 times during Spyder startup!).

@mrclary mrclary self-assigned this Dec 14, 2024
@mrclary mrclary added this to the v6.0.4 milestone Dec 15, 2024
@mrclary mrclary marked this pull request as ready for review December 15, 2024 04:32
@mrclary mrclary force-pushed the env-var-timeout branch 3 times, most recently from b9c2b90 to 8d1c172 Compare December 17, 2024 07:19
@ccordoba12 ccordoba12 modified the milestones: v6.0.4, v6.0.5 Jan 29, 2025
@mrclary mrclary marked this pull request as draft February 18, 2025 20:31
@mrclary

mrclary commented Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

@ccordoba12, I think you mentioned using spyder.api.asyncdispatcher for this PR; was that correct? If so, I may need some guidance on that.

Comment thread spyder/plugins/ipythonconsole/widgets/main_widget.py Outdated
Comment thread spyder/plugins/ipythonconsole/widgets/main_widget.py Outdated
Comment thread spyder/plugins/ipythonconsole/widgets/main_widget.py Outdated
Comment thread spyder/plugins/ipythonconsole/widgets/main_widget.py Outdated
Comment thread spyder/utils/environ.py Outdated
@mrclary mrclary force-pushed the env-var-timeout branch 2 times, most recently from 130519b to d4cc4dc Compare March 6, 2025 22:09
@mrclary

mrclary commented Mar 8, 2025

Copy link
Copy Markdown
Contributor Author

#21769 introduced running the user_env.sh script in pytests. Without it, get_user_environment_variables cannot be fully tested on CI. While this works on CI, locally run pytests (at least for me) that use the ipyconsole fixture get a SIGSTP somehow. Probably because local tests are run in an interactive shell, while CI is not (see #22415). Exchanging running_under_pytest for running_in_ci should fix this.

Now, specific to this PR, failing tests:

  • test_old_kernel_version: kernel_handler.check_spyder_kernel_info(('1.0.0', '')) appears ineffective; a new client is successfully created
  • test_dedicated_consoles: appears to be a race condition. Sometimes qtbot.waitUntil(lambda: nsb.editor.source_model.rowCount() == 4) times out even though there are 4 entries; sometimes a %clear command is issued with the F5 key so that the banner is not captured and assert ('runfile' in text) and ('Python' in text and 'IPython' in text) fails. 🤷🏼 . Perhaps moving get_user_environment_variables out of the property SpyderKernelSpec.env changed its performance (for the better) but then messed with the timing in this test.

I don't believe there is any issue with moving get_user_environment_variables. I think the issue is elsewhere. Perhaps a race condition with some Qt signals in the run or editor plugins, and the kernel handler.

@ccordoba12 ccordoba12 modified the milestones: v6.0.5, v6.0.6 Mar 11, 2025
@mrclary

mrclary commented Mar 11, 2025

Copy link
Copy Markdown
Contributor Author

The reason the tests were failing was due to checking the kernel cache, which compares all attributes of the SpyderKernelSpec. The new private _env_vars attribute was causing the comparison to fail and the solution was to ensure that PYTEST_CURRENT_TEST key was removed from _env_vars.

test_dedicated_console still has issues on my local macOS machine, but not on my Linux VM, so it may be macOS intel related. Nevertheless, it seems to be working on CI now, so I'll leave well enough alone.

@mrclary mrclary marked this pull request as ready for review March 11, 2025 22:50

@ccordoba12 ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small suggestions for you @mrclary, the rest looks good to me.

Comment thread spyder/app/tests/test_mainwindow.py Outdated
Comment thread spyder/utils/environ.py Outdated
@ccordoba12 ccordoba12 modified the milestones: v6.0.6, v6.0.5 Mar 13, 2025
mrclary and others added 5 commits March 20, 2025 08:31
…an interactive shell that results in a SIGSTP.
Timeouts do not seem to be honored. ctrl-c still indicates test_dedicated_consoles is hanging at "with qtbot.waitSignal(shell.executed".
…ironment_variables once at instantiation, rather than every time SpyderKernelSpec.env is referenced. This should improve performance of starting new consoles since user_env.sh will not be executed as frequently.

A setter is required in order to remove PYTEST_CURRENT_TEST from the meta property SpyderKernelSpec._env_vars, because CachedKernelMixin compares SpyderKernelSpec.__dict__ to validate cache.
…t is to guard against indefinite hangs; most users will never experience this limit.
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12 ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks @mrclary!

@ccordoba12 ccordoba12 changed the title PR: Raise timeout threshold for getting user environment variables PR: Reduce calls to get environment variables in SpyderKernelSpec and raise timeout threshold to do that Mar 20, 2025
@ccordoba12 ccordoba12 merged commit 9314704 into spyder-ide:master Mar 20, 2025
@ccordoba12

Copy link
Copy Markdown
Member

@meeseeksdev please backport to 6.x

@lumberbot-app

lumberbot-app Bot commented Mar 20, 2025

Copy link
Copy Markdown

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 6.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 9314704ed5b6bca4b75ba7f41b849349fb8c97c0
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #23278: PR: Reduce calls to get environment variables in `SpyderKernelSpec` and raise timeout threshold to do that '
  1. Push to a named branch:
git push YOURFORK 6.x:auto-backport-of-pr-23278-on-6.x
  1. Create a PR against branch 6.x, I would have named this PR:

"Backport PR #23278 on branch 6.x (PR: Reduce calls to get environment variables in SpyderKernelSpec and raise timeout threshold to do that )"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@ccordoba12 ccordoba12 changed the title PR: Reduce calls to get environment variables in SpyderKernelSpec and raise timeout threshold to do that PR: Reduce calls to get environment variables in SpyderKernelSpec and raise timeout threshold to do that Mar 20, 2025
ccordoba12 added a commit to ccordoba12/spyder that referenced this pull request Mar 20, 2025
…iables in `SpyderKernelSpec` and raise timeout threshold to do that
ccordoba12 added a commit that referenced this pull request Mar 20, 2025
… variables in SpyderKernelSpec and raise timeout threshold to do that) (#24001)
@mrclary mrclary deleted the env-var-timeout branch March 21, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants