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

PR: Disable autosave if not running in single instance mode #9660

Merged
merged 2 commits into from
Jun 25, 2019

Conversation

jitseniesen
Copy link
Member

@jitseniesen jitseniesen commented Jun 23, 2019

This is a temporary stop gap to work around bugs in the autosave system like #9420 when multiple Spyder instances are running simultaneously. Once a more permanent fix is developed, this can be reverted.

I did not include a test, because the code is already run by existing tests and it seems a bit too much effort to write a test for what is meant to be a temporary change,

Issue(s) Resolved

This does not truly resolve the issue, so no "Fixes #..."

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: Jitse Niesen

@jitseniesen jitseniesen added this to the v4.0beta3 milestone Jun 23, 2019
@jitseniesen jitseniesen changed the title Disable autosave if not running in single instance mode. PR: Disable autosave if not running in single instance mode Jun 23, 2019
@jitseniesen
Copy link
Member Author

To clarify, I am submitting a temporary fix so that the issue does not block the release of v4.0 beta 3.

@jitseniesen
Copy link
Member Author

Don't be lazy, Jitse, just add a test!

@jitseniesen
Copy link
Member Author

As I understand it, the test failures are due to on-going work on the Object Explorer and not related to this PR.

@ccordoba12
Copy link
Member

Yes, that's right. I'm working right now to finish PR #8852 so our tests pass again. I'll let you know when that's done.

@jitseniesen jitseniesen force-pushed the disable-autosave branch 2 times, most recently from 6c7eb33 to 7062730 Compare June 25, 2019 09:59
@pep8speaks
Copy link

pep8speaks commented Jun 25, 2019

Hello @jitseniesen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-25 13:51:04 UTC

@ccordoba12
Copy link
Member

Please merge with master to get a new set of fixes for our tests.

This is a temporary stop gap to work around bugs in the autosave
system when multiple Spyder instances are running simultaneously
(e.g., issue spyder-ide#9420). Once a more permanent fix is developed, this
can be reverted.
@jitseniesen
Copy link
Member Author

The test test_get_hints[params4] in spyder/plugins/editor/widgets/tests/test_hints_and_calltips.py failed consistently, but only on Python 3.6 in Windows on Azure. I think it is unlikely to be related to this PR, so I don't intend to investigate it further.

@ccordoba12
Copy link
Member

That's fine @jitseniesen, don't worry about it. Unfortunately those tests are very flaky.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jitseniesen for your help with this one!

@ccordoba12 ccordoba12 merged commit 1c795a7 into spyder-ide:master Jun 25, 2019
@jitseniesen jitseniesen deleted the disable-autosave branch July 4, 2019 11:28
jitseniesen added a commit to jitseniesen/spyder that referenced this pull request Aug 23, 2019
…utosave"

The issue that the autosave system does not work correctly with multiple
Spyder instances is about to be fixed, so no need anymore to turn it off
when not run in single instance mode.

This reverts commit 1c795a7, reversing
changes made to ad654be.
jitseniesen added a commit to jitseniesen/spyder that referenced this pull request Aug 25, 2019
…utosave"

The issue that the autosave system does not work correctly with multiple
Spyder instances is about to be fixed, so no need anymore to turn it off
when not run in single instance mode.

This reverts commit 1c795a7, reversing
changes made to ad654be.
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Aug 27, 2019

For what its worth, as I stated on #9420 ,

to ignore errors here as well and resolve this issue, just pass errors="ignore" to remove_autosave_file() on Line 1521 of spyder/plugins/editor/widgets/editor.py.

This would avoid completely silently and invisibly disabling the autosave safety net if and when a user had disabled single instance mode, as I normally always do. As a result of this PR, over the past month I've lost a total of several hours of work over the dozens of times when Spyder hangs seemingly at random every few hours. Until I realized it could not be mere bad luck not having autosaves and poured line by line through the verbose mode logs to figure out what was really the problem, I had no way of knowing why.

There's no warning on startup, no warning when enabling or disabling single instance mode, no warning when enabling or disabling autosaves, autosaves still shows as enabled in the preferences, and nothing in the UI or tooltip text of either option to indicate this major and potentially crippling consequence. As a user, if I didn't already have it happen to me and lose work, know how to enable verbose logging and exactly what to look for it there, and then know how and where to search for this PR (i.e. if I wasn't part of the 0.1% of Spyder's userbase that was a contributor/developer), it would be essentially impossible to figure out why this was happening.

This might be excusable if it was only in the Github builds for a short time, but this has made it into not one but two public betas so far. I know I don't have any credibility around here anymore, but if you can remember one thing from me, please try to carefully think through the UX implications of your changes. Whiz bang features may attract users initially, but its good UX that keeps them coming back over the long term.

@jitseniesen
Copy link
Member Author

It is not only remove_autosave_files that causes issues. PR #10077 is a permanent fix for the issue.

@ccordoba12
Copy link
Member

This might be excusable if it was only in the Github builds for a short time, but this has made it into not one but two public betas so far

Betas are betas, i.e. they are not expected to be stable nor the final version. So I don't agree with making the extra effort of informing users that autosaving doesn't work with multiple instances when that's going to be fixed before the final release.

@CAM-Gerlach
Copy link
Member

It is not only remove_autosave_files that causes issues. PR #10077 is a permanent fix for the issue.

Yes, of course, but the broader point is that silently and invisibly rendering autosave completely disabled without warning, notification or any indication whatsoever (even a still indicating it as enabled in preferences) is a much more impactful issue for the group of users without single instance mode enabled than any of the originals were. Those issues were a nuisance, while this one is a potential disaster.

@CAM-Gerlach
Copy link
Member

So I don't agree with making the extra effort of informing users that autosaving doesn't work with multiple instances when that's going to be fixed before the final release.

If the betas were only used by the very small population of contributors and developers capable of knowing all the above steps in order to figure out why autosave suddenly stopped working, as opposed to a much wider user population likely to be confused and frustrated by this inexplicable behavior, then they serve little purpose as those are the very users already (or at least capable of) running straight from a Github clone. However, the 27 629 total downloads of Spyder from the spyder-ide beta channel would seem to suggest otherwise.

Furthermore, unless doing so would take more than a few hours to implement, it would already would have been worthwhile in terms of avoiding wasted time, effort and frustration this cost one single user (myself) out of at least thousands. It would take only a few minutes to at the very least tweak the UI text/tooltips of these options to reflect the change and mention something in the beta release notes, (although inexplicably there no longer appears to be any summary of the major changes in each beta), and I would be in bed asleep right now instead of still at the lab complaining about this to you.

@ccordoba12
Copy link
Member

However, the 27 629 total downloads of Spyder from the spyder-ide beta channel would seem to suggest otherwise.

Compared to the ~200k from Anaconda + ~40K from PyPi downloads per month, that's certainly negligible. And I'm sorry if those users feel frustrated but that's the chance they take by using a beta.

And again, betas are really for experimentation without disrupting our entire user base. So please don't insist on us working to make our betas production ready.

Furthermore, unless doing so would take more than a few hours to implement, it would already would have been worthwhile in terms of avoiding wasted time, effort and frustration this cost one single user (myself) out of at least thousands

If you felt so concerned about this, I don't understand why you didn't take the time to add this yourself instead of asking us to do it.

Final note: I'm locking this PR because it makes no sense to keep discussing about this.

@spyder-ide spyder-ide locked as resolved and limited conversation to collaborators Aug 27, 2019
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.

4 participants