Skip to content

Change check_freq default to False #35571

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

Closed
wants to merge 1 commit into from
Closed

Change check_freq default to False #35571

wants to merge 1 commit into from

Conversation

MaxWinterstein
Copy link

@jbrockmendel
Copy link
Member

This effectively disables a bunch of existing checks. passing check_freq=True explicitly in a ton of places is an option i guess. or could change the default to False and warn that it will become True in a future version

@simonjayhawkins simonjayhawkins added this to the 1.1.1 milestone Aug 11, 2020
@simonjayhawkins simonjayhawkins added Regression Functionality that used to work in a prior pandas version Testing pandas testing functions or related to the test suite labels Aug 11, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

we need to leave this as true as its more strict; sure it might have broken downstream, but not testing things is not great, so -1 on this PR

@jreback jreback removed this from the 1.1.1 milestone Aug 14, 2020
@TomAugspurger
Copy link
Contributor

And FYI, this came up on the dev call on Wednesday. The consensus was roughly "we didn't mean to change this for 1.1.0, but now that we have it's best to just leave it."

Apologies for the accidental breaking change @MaxWinterstein, and thanks for working on the PR but we'll pass.

In the future, you might want to subscribe to pandas releases by "watching" the repo for "releases only". We make binaries available for release candidates so it should be easy to test.

@MaxWinterstein
Copy link
Author

And FYI, this came up on the dev call on Wednesday. The consensus was roughly "we didn't mean to change this for 1.1.0, but now that we have it's best to just leave it."

Apologies for the accidental breaking change @MaxWinterstein, and thanks for working on the PR but we'll pass.

In the future, you might want to subscribe to pandas releases by "watching" the repo for "releases only". We make binaries available for release candidates so it should be easy to test.

First of all, i am totally fine by keeping it that way. No hate.

I was/am just a little unhappy that this is - in my opinion - a bigger change that should be mentioned in the change logs anywhere. But afaik there is no mention at all.
As our automated tests on ou rCI failed successfully, there was no hassle or problem on production side, as we keep requirements versioned (as everybody should 😉 ). But when you try to update your requirements and you need to dig inside the projects to find why something is suddenly broken, its a little bit frustrating...

@TomAugspurger
Copy link
Contributor

For sure. This was a process failure where we merged a breaking API change to master. That was noticed during review, but a followup issue was never opened.

As I mentioned in the issue, updating the release notes to explain this API change may still be worthwhile since it might help future upgraders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Regression Functionality that used to work in a prior pandas version Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the default of check_freq to False as it breaks existing code
5 participants