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

Invalidate all cache files if plugins change #5878

Merged
merged 6 commits into from Nov 14, 2018

Conversation

Projects
None yet
2 participants
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Nov 8, 2018

Fixes #3592

This invalidates all cache files if at least one plugin changes its __version__ or hash of its content (this will not catch situations when there are changes in modules imported by a plugin, and version is also imported, but such situations are probably very rare).

I was not sure where to place the check to invalidate all caches at the same time, so I invalidate them one by one.

@ilevkivskyi ilevkivskyi requested a review from JukkaL Nov 8, 2018

@JukkaL
Copy link
Collaborator

JukkaL left a comment

Nice, this will make plugins behave more reliably.

@@ -375,11 +379,21 @@ def plugin_error(message: str) -> None:
'(in {})'.format(plugin_path))
try:
custom_plugins.append(plugin_type(options))
# We record _both_ hash and the version to detect more

This comment has been minimized.

@JukkaL

JukkaL Nov 9, 2018

Collaborator

This function is quite long. It would be better if this would be moved to a helper method to avoid making it even longer.

@@ -5389,3 +5393,58 @@ E = Enum('E', f()) # type: ignore
[builtins fixtures/list.pyi]
[out]
[out2]

[case testChangedPluginsInvalidateCache]

This comment has been minimized.

@JukkaL

JukkaL Nov 9, 2018

Collaborator

Maybe add another test where only the version changes (e.g. import version from another module so that the text of the plugin doesn't change).

@@ -487,6 +501,9 @@ def __init__(self, data_dir: str,
in self.options.shadow_file}
# a mapping from each file being typechecked to its possible shadow file
self.shadow_equivalence_map = {} # type: Dict[str, Optional[str]]
self.plugin = plugin
self.plugins_snapshot = plugins_snapshot

This comment has been minimized.

@JukkaL

JukkaL Nov 9, 2018

Collaborator

Document the new attributes in the class docstring.


[case testChangedPluginsInvalidateCache]
# flags: --config-file tmp/mypy.ini
import a

This comment has been minimized.

@JukkaL

JukkaL Nov 9, 2018

Collaborator

Would it make sense to add a fine-grained incremental test case where a plugin changes?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 9, 2018

Collaborator

As we discussed, this will have no effect.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 9, 2018

Collaborator

Hm, I have found there is another daemon command dmypy run, which unlike dmypy check restarts if options change. I think we can also restart in dmypy run if plugins change. There is however no way to test this currently.

Ivan Levkivskyi
@JukkaL

This comment has been minimized.

Copy link
Collaborator

JukkaL commented Nov 9, 2018

There is a test failure.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 9, 2018

Hm, this is weird, all tests pass locally. Also the error appears on the first run, which is even more weird.

Ivan Levkivskyi added some commits Nov 9, 2018

Ivan Levkivskyi
Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 10, 2018

OK, I also added restart for daemon if plugins change (in addition to cache invalidation) and tested manually that it works. @msullivan does this make sense to you?

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 13, 2018

@msullivan @JukkaL I assume this looks good now. Are there any other comments?

@@ -848,6 +901,11 @@ def find_cache_meta(id: str, path: str, manager: BuildManager) -> Optional[Cache
manager.trace(' {}: {} != {}'
.format(key, cached_options.get(key), current_options.get(key)))
return None
if manager.old_plugins_snapshot and manager.plugins_snapshot:

This comment has been minimized.

@JukkaL

JukkaL Nov 13, 2018

Collaborator

What about adding a test for this?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 14, 2018

Collaborator

We can't yet add tests for daemon. I opened #5891 for this.

@ilevkivskyi ilevkivskyi merged commit 74c5070 into python:master Nov 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:invalidate-plugin-change branch Nov 14, 2018

ilevkivskyi added a commit that referenced this pull request Nov 14, 2018

Invalidate cache when a plugin is added/removed (#5892)
#5878 added cache invalidation for situations where a plugin version/hash changes but there is even simpler scenario -- if `plugins` option itself change, the cache should be invalidated as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment