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

Require explicit flag to remove MyPy cache when running breeze stop #29493

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Feb 12, 2023

The change #29184 added cleanup of MyPy cache when breeze stop was called, but this is a bit too agressive. It will remove the cache always when breeze stop is run and it will prolong (on MacOS by 10s of seconds) the MyPy static checks when it happens.

Since cleaning the cache is only really needed when MyPy cache gets broken (for example when we upgrade MyPy), cleaning it explicitly when you encounter such problem is a better idea.

This PR:

  • adds flag to breeze stop to clean the mypy cache (disabled by default)
  • adds documentation about that feature in STATIC_CHECKS
  • adds hint displayed to the user when mypy check fails

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

The change apache#29184 added cleanup of MyPy cache when `breeze stop`
was called, but this is a bit too agressive. It will remove
the cache always when breeze stop is run and it will prolong
(on MacOS by 10s of seconds) the MyPy static checks when it happens.

Since cleaning the cache is only really needed when MyPy cache
gets broken (for example when we upgrade MyPy), cleaning it
explicitly when you encounter such problem is a better idea.

This PR:

* adds flag to breeze stop to clean the mypy cache (disabled by
  default)
* adds documentation about that feature in STATIC_CHECKS
* adds hint displayed to the user when mypy check fails
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Breeze is aware of when library version changes as it asks to rebuild the image can we maybe detect mypy version changes and then promt the question to clean the cache (or just cleanit anyway under the hood)?

@potiuk
Copy link
Member Author

potiuk commented Feb 12, 2023

Well. It's not 'Always' when mypy version changes. In vast majority of cases cache worked fine when we upgraded mypy. Usually mypy can clean it up all right. It was just one case when some cache was broken few weeks ago that affected literally few people i am aware of. And also the cache could be broken for other reasons (for example when you ctrl-c while mypy doing it's job if you are unlucky/. So this is really 'easy recovery' when things get really, really wrong

@potiuk
Copy link
Member Author

potiuk commented Feb 12, 2023

I am also not even sure if the root cause in this case was related to MyPy upgrade actually - it looked like it could be (because that was around the time when we upgraded), but I cannot be 100% sure 🤷 . It could also be because - for example - some typeshed changed or mypy information for our dependencies and some stored information in cache could be wrong and "missed" by MyPy (and that one would be next to impossible to know where such error gets triggered).

So we do not really have a clear "trigger" for cleaning the cache that we are sure of. We could potentially catch the right exception and clean it up, but this kind of error might result in an exception that we won't be able to detect as "invalid cache" being the root cause.

Best we can do (and I did) is revert to human-in-the-loop and in case we see an error tell the human "look - if you see strange errors, maybe this one will help".

@potiuk potiuk merged commit 8841537 into apache:main Feb 12, 2023
@potiuk potiuk deleted the clear-mypy-volume-optionally-on-breeze-stop branch February 12, 2023 17:01
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
…29493)

The change #29184 added cleanup of MyPy cache when `breeze stop`
was called, but this is a bit too agressive. It will remove
the cache always when breeze stop is run and it will prolong
(on MacOS by 10s of seconds) the MyPy static checks when it happens.

Since cleaning the cache is only really needed when MyPy cache
gets broken (for example when we upgrade MyPy), cleaning it
explicitly when you encounter such problem is a better idea.

This PR:

* adds flag to breeze stop to clean the mypy cache (disabled by
  default)
* adds documentation about that feature in STATIC_CHECKS
* adds hint displayed to the user when mypy check fails

(cherry picked from commit 8841537)
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
…29493)

The change #29184 added cleanup of MyPy cache when `breeze stop`
was called, but this is a bit too agressive. It will remove
the cache always when breeze stop is run and it will prolong
(on MacOS by 10s of seconds) the MyPy static checks when it happens.

Since cleaning the cache is only really needed when MyPy cache
gets broken (for example when we upgrade MyPy), cleaning it
explicitly when you encounter such problem is a better idea.

This PR:

* adds flag to breeze stop to clean the mypy cache (disabled by
  default)
* adds documentation about that feature in STATIC_CHECKS
* adds hint displayed to the user when mypy check fails

(cherry picked from commit 8841537)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants