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

Delete cache for a module if errors are found #4045

Merged
merged 3 commits into from Oct 4, 2017

Conversation

Projects
None yet
2 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Oct 2, 2017

Fixes #4043
Fixes #3852
Fixes #3052

This is a simple-minded solution for inconsistent cache state problem discovered in #4043.

Show outdated Hide outdated mypy/build.py Outdated
@@ -1813,6 +1831,7 @@ def write_cache(self) -> None:
else:
is_errors = self.manager.errors.is_errors()
if is_errors:
delete_cache(self.id, self.path, self.manager)

This comment has been minimized.

@gvanrossum

gvanrossum Oct 2, 2017

Member

Shouldn't this also mark the interface as stale (as below on line 1845-1847) and set self.interface_hash to something unmatchable? Otherwise I worry we could still repro the same issues by adding another chain to the reference link.

@gvanrossum

gvanrossum Oct 2, 2017

Member

Shouldn't this also mark the interface as stale (as below on line 1845-1847) and set self.interface_hash to something unmatchable? Otherwise I worry we could still repro the same issues by adding another chain to the reference link.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 3, 2017

Collaborator

Yes, you are right. I even have found a longer crasher with four files that still happens unless interface is marked as stale (I added it to the tests). Do we also need to set the interface_hash to something? It looks like it is not necessary.

It looks like marking files with errors as stale breaks several tests. I will check if this can be fixed somehow.

@ilevkivskyi

ilevkivskyi Oct 3, 2017

Collaborator

Yes, you are right. I even have found a longer crasher with four files that still happens unless interface is marked as stale (I added it to the tests). Do we also need to set the interface_hash to something? It looks like it is not necessary.

It looks like marking files with errors as stale breaks several tests. I will check if this can be fixed somehow.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 3, 2017

Collaborator

Hm, there is an explicit comment in tests:

Note that a file that ends up producing an error does not create
a new cache file and so is not considered stale.

Should I just mark them as stale now and update this comment?

@ilevkivskyi

ilevkivskyi Oct 3, 2017

Collaborator

Hm, there is an explicit comment in tests:

Note that a file that ends up producing an error does not create
a new cache file and so is not considered stale.

Should I just mark them as stale now and update this comment?

This comment has been minimized.

@gvanrossum

gvanrossum Oct 3, 2017

Member

Yeah, I think the comment just reflects the state of the world at the time (i.e. that no cache file was written).

I presume this only affects some of the tests and not all tests with a [stale] marker?

Alternatively you could have a flag to mark_interface_stale() to skip adding the module to stale_modules since that is used purely by the tests. (The other two things it sets are used for the graph algorithm too.)

@gvanrossum

gvanrossum Oct 3, 2017

Member

Yeah, I think the comment just reflects the state of the world at the time (i.e. that no cache file was written).

I presume this only affects some of the tests and not all tests with a [stale] marker?

Alternatively you could have a flag to mark_interface_stale() to skip adding the module to stale_modules since that is used purely by the tests. (The other two things it sets are used for the graph algorithm too.)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 3, 2017

Collaborator

I presume this only affects some of the tests and not all tests with a [stale] marker?

It affects 42 tests, i.e. around one third of all tests with [stale].

Alternatively you could have a flag to mark_interface_stale() to skip adding the module to stale_modules since that is used purely by the tests. (The other two things it sets are used for the graph algorithm too.)

OK, I did this. Now all tests should pass. By the way, what do you think about potential performance issues because of this change? Intuitively it should be minor (since it only affects situations where new errors appear after previous incremental runs), but maybe you could measure it somehow?

@ilevkivskyi

ilevkivskyi Oct 3, 2017

Collaborator

I presume this only affects some of the tests and not all tests with a [stale] marker?

It affects 42 tests, i.e. around one third of all tests with [stale].

Alternatively you could have a flag to mark_interface_stale() to skip adding the module to stale_modules since that is used purely by the tests. (The other two things it sets are used for the graph algorithm too.)

OK, I did this. Now all tests should pass. By the way, what do you think about potential performance issues because of this change? Intuitively it should be minor (since it only affects situations where new errors appear after previous incremental runs), but maybe you could measure it somehow?

ilevkivskyi added some commits Oct 3, 2017

@gvanrossum gvanrossum merged commit 51fb765 into python:master Oct 4, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Oct 4, 2017

Member

Whee!

Member

gvanrossum commented Oct 4, 2017

Whee!

@gvanrossum gvanrossum referenced this pull request Oct 4, 2017

Closed

Release 0.530 planning #4009

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment