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

Ignore failed move of cache index and warn. #1235

Merged
merged 1 commit into from
Feb 17, 2017
Merged

Ignore failed move of cache index and warn. #1235

merged 1 commit into from
Feb 17, 2017

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Dec 13, 2016

Motivation and context:
The replace respectively move can fail on Windows due to some interaction with anti-virus software (see #1200 for details). This PR ignores the failure and produces a warning.

Interactions with other PRs:
I would like to include a link to some documentation on how to disable the cache in the warning, but I need #1130 to be merged for that.

How has this been tested?
I haven't tested this because testing stuff on Windows is always very time consuming and annoying for me. But maybe @arvoelke is so nice to give it a test run? 🙏

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • [n/a] I have updated the documentation accordingly.
  • I have included a changelog entry.
  • [n/a] I have added tests to cover my changes. (Hard to do because the code in question is only used on very specific systems and the problem can be only reproduced under certain conditions.)
  • All new and existing tests passed.

@jgosmann
Copy link
Collaborator Author

It would probably be nice to have this in a 2.3.1 bugfix release.

@Seanny123
Copy link
Contributor

LGTM, pending on this working on Aaron's system which experienced the problem in the first place.

@arvoelke
Copy link
Contributor

c:\users\aaron\ctn\nengo\nengo\cache.pyc in _write_index(self)
    404             # moved. There is not a lot we could do about this. See
    405             # <https://github.com/nengo/nengo/issues/1200> for more info.
--> 406             warnings.warn(CacheIndexReplaceFailedWarning(self))
    407         if os.path.exists(self.legacy_path):
    408             os.remove(self.legacy_path)

TypeError: expected string or buffer

@jgosmann jgosmann self-assigned this Dec 14, 2016
@jgosmann jgosmann removed their assignment Dec 14, 2016
@jgosmann
Copy link
Collaborator Author

Made a fixup commit, hopefully it works now.

@arvoelke
Copy link
Contributor

It worked. 🎆 Do we have an open issue for documenting how to disable the cache? Point 3 in the warning is currently not so helpful for the average user since googling "nengo disable cache" or searching our pydocs for "cache disable" brings up nothing.

@jgosmann
Copy link
Collaborator Author

👉 Please read “Interactions with other PRs” of the PR description.

@tbekolay
Copy link
Member

Pushed some commits to rearrange a bit and add a test. If they're OK with @jgosmann I'll merge; will add a possible 2.3.1 release to the dev meeting agenda.

@jgosmann
Copy link
Collaborator Author

Looks fine to me. Not sure if the added test is necessary or useful. It seems to test implementation details to some degree (whether replace will actually be used) and it almost monkey-patches in the desired behaviour (though, I suppose it is still testing that the exception is converted to a warning).

@jgosmann
Copy link
Collaborator Author

@tbekolay friendly reminder to consider this for inclusion in 2.3.1

Also, there is PR #1264 which seems to do about the same thing? Not sure why I opened that one ... maybe I just forgot that I already fixed the problem ...?

@tbekolay
Copy link
Member

Thanks for the ping, will look at this now, thanks!

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

Looks good! I'll add the changelog entry in the merge.

@tbekolay tbekolay merged commit 930041f into master Feb 17, 2017
@tbekolay tbekolay deleted the failsafe-move branch February 17, 2017 16:29
@tbekolay tbekolay mentioned this pull request Feb 17, 2017
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants