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

Fix add_to_spyder_UMR_excludelist for Spyder 6 #6544

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sldesnoo-Delft
Copy link
Contributor

The import of qcodes fails with Spyder 6, because add_to_spyder_UMR_excludelist raises an exception.

This pull request fixes the code to work with Spyder 6.0.
I've removed a bit of old code that was only useful for Spyder 3.x

I don't think this is easy to test with an automatic test, because a test would require Spyder 6.

@sldesnoo-Delft sldesnoo-Delft requested a review from a team as a code owner October 18, 2024 15:54
@sldesnoo-Delft
Copy link
Contributor Author

@microsoft-github-policy-service agree

else:
sitecustomize_found = True
# In Spyder 6 UMR is an attribute of the SpyderCodeRunner object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I also encountered qcodes not running with the new Spyder 6. The UserModuleReloader is also available via a normal import:

import spyder_kernels.customize.umr; spyder_kernels.customize.umr.UserModuleReloader

Using that instead of sitecustomize.UserModuleReloader works for me so far.

@ccordoba12 Do you have an opinion on how to approach this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that as well.
Unfortunately Spyder6 does not use the global umr. So assignment of the changed UMR has no effect.

That's why I searched for the UMR instance that is being used and modify that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eendebakpt @sldesnoo-Delft I am not using spyder my self at the moment so unlikely to spot this.

At some point a long time ago I did look into addressing this issue on the spyder side spyder-ide/spyder#2451 If anyone have bandwidth to do that it would be great as this is a much cleaner option.

Otherwise happy to merge this once you have converted on the best solution

Copy link

@ccordoba12 ccordoba12 Oct 23, 2024

Choose a reason for hiding this comment

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

@ccordoba12 Do you have an opinion on how to approach this?

I wasn't aware people was adding modules to our UMR programmatically (no one has reported it to our issue tracker).

Since the code to do that for Spyder 6/Spyder-kernels 3 is too convoluted (from what I can see below), we could add a public API at the shell level (i.e. the object you get with get_python()) to make the life of other projects easier.

@jenshnielsen, @sldesnoo-Delft, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

qcodes adds itselft to the list of packages to exclude.

I had a closer look at the UMR. It checks if the module is in the library path. (path_is_library in utils.py). When qcodes is normally installed, i.e. not in "develop mode", then it will not be reloaded by UMR.

So, I believe the add_to_spyder_UMR_excludelist function is not really needed. It might be a remainder from a distant past.

@jenshnielsen , @eendebakpt do you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sldesnoo-Delft I think this has ever only been a problem with editable installs. As I recall the problem is that when spyder does reload all singletons are destroyed so instruments are lost. I think that when we originally added this qcodes did not see regular releases, so a lot of users were installing from source. This is much less of an issue now so I think it would be fine to deprecate/remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no strong opinion on this, we usually disable the spyder UMR (other packages have issues with automatic reloading as well). We only noticed because the new spyder 6 would not import qcodes.

If we do remove the functionality, I do think it warrants some attention in the release notes (and maybe the documentation?).

@jenshnielsen jenshnielsen added this to the 0.50.0 milestone Nov 11, 2024
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 3.70370% with 26 lines in your changes missing coverage. Please review.

Project coverage is 68.24%. Comparing base (ef66ccc) to head (5efe1e4).
Report is 148 commits behind head on main.

Files with missing lines Patch % Lines
src/qcodes/utils/spyder_utils.py 3.70% 26 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6544      +/-   ##
==========================================
- Coverage   69.33%   68.24%   -1.10%     
==========================================
  Files         340      340              
  Lines       31261    31268       +7     
==========================================
- Hits        21676    21340     -336     
- Misses       9585     9928     +343     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

4 participants