Remove -Werror from setup.py
#33315
Comments
xhochy
added
Bug
Needs Triage
3 tasks
simonjayhawkins
added
Build
|
Works for me |
|
Unless there is an urgent need for this I would prefer not to. Haven't looked into the latest issue you have opened, but of anything else to date they are upstream bugs that we have since raised back with those projects; we wouldn't have visibility to those otherwise |
We would still catch these on CI by setting |
Those issues weren't discovered by CI but rather by contributors, so if we do what I think is suggested here we won't discover those issues. Understood on packaging but is there not a concrete set of warnings we can ignore via |
|
OK. I haven't followed the issues too closely. While I'm here, I'm trying to run ASV on a branch but I'm seeing How would I debug that? |
|
That warning is thrown during the Cythonize step not compilation, so slightly separate from this.
I think the cythonize command accepts Werror, but we don’t pass it and there’s a lot of work to be done for latest bug fix release to clean warnings. Not sure how to best debug
…Sent from my iPhone
On Apr 7, 2020, at 4:20 AM, Tom Augspurger ***@***.***> wrote:
OK. I haven't followed the issues too closely.
While I'm here, I'm trying to run ASV on a branch but I'm seeing
gcc -bundle -undefined dynamic_lookup -L/Users/taugspurger/sandbox/pandas-asv/asv_bench/env/40d026129f63c107b1ac48bf9ad6964c/lib -arch x86_64 -L/Users/taugspurger/sandbox/pandas-asv/asv_bench/env/40d026129f63c107b1ac48bf9ad6964c/lib -arch x86_64 -arch x86_64 build/temp.macosx-10.9-x86_64-3.6/pandas/_libs/join.o -o build/lib.macosx-10.9-x86_64-3.6/pandas/_libs/join.cpython-36m-darwin.so
STDERR -------->
warning: pandas/_libs/groupby.pyx:1097:26: Unreachable code
How would I debug that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Ah gotcha. So even though that's warning it's not causing the failure. My failure seems to come from the use of a deprecated macro create environment build There is a warning while cythonizing The errors |
|
My workaround for now is to set Is it clear to anyone else who's using |
|
You are on a Mac right? There’s a separate issue specifically with 3.8 for tp_print.
That’s a definitely a weird one. Maybe we can change upstream in Cython. If not could add a no-error flag for deprecation warnings
…Sent from my iPhone
On Apr 7, 2020, at 1:37 PM, Tom Augspurger ***@***.***> wrote:
My workaround for now is to set CFLAGS=-Wno-error=deprecated-declarations prior to building extensions.
Is it clear to anyone else who's using tp_print? Us (in algos.pyx)? Cython? Python?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Seems reasonable. I think only needed for Py38 (removed in 39)
…Sent from my iPhone
On Apr 7, 2020, at 1:46 PM, Tom Augspurger ***@***.***> wrote:
Yeah, on a mac. #33239 is the issue you were thinking of.
In our setup.py we currently skip -Werror for windows. Perhaps we add -Wno-error=deprecated-declarations for MacOS, while we work out issues with -Werror?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
FWIW, I think my struggles in MacPython/pandas-wheels#85 are a preview of what downstream packagers will face with this. Presumably those packagers are more familiar with building C/C++ code than I am, but I suspect it will still cause issues. |
5 tasks
|
Maybe having issues with this for the 1.1.0rc0: conda-forge/pandas-feedstock#84 (comment). Trying to patch around it for now but we should revisit before the final. |
|
There's also a bunch of new deprecations in Python 3.9: |
|
I am trying to use pandas with python3.9, and noticed this issue: With over 3k open issues, I am not sure trying to find every possible warning of every possible compiler out there is a good idea. The general idea of warnings is that the compiler isn't sure the code is wrong - but only if it is certain the code is wrong it will raise an error. |
3 tasks
5 tasks
5 tasks
|
@WillAyd are you still against removing the hardcoded (I don't have this problem myself, but it seems lots of people run into it (including ourselves when building wheels/conda packages), so it would be nice to make this a better experience) |
|
I think we should still keep in setup.py - we’ve been doing a lot of work elsewhere to make local development and CI match so would like to maintain that
…Sent from my iPhone
On Nov 13, 2020, at 5:32 AM, Joris Van den Bossche ***@***.***> wrote:
@WillAyd are you still against removing the hardcoded -Wno-error from our setup.py? (while keeping it in CI) Or is the conclusion to use -Wno-error=deprecated-declarations ? (looking at #33315 (comment))
(I don't have this problem myself, but it seems lots of people run into it (including ourselves when building wheels/conda packages), so it would be nice to make this a better experience)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
For me this is a really big annoyance that I always need to remove it when I work on pandas but I guess new contributors will just be discouraged to even look into contributing when they don't get it to build locally. Removing Also from the packager side: It is always a big annoyance when you need a patch to build a package that upstream doesn't accept. Especially as it seems that this patch is applied in any pandas distribution. |
|
I don’t really buy that this discourages new contributors. On most platforms there are no issues, and we’ve added a few new supported platforms since this has been enabled (all from new contributors). It gives a good baseline that we can actually support a platform and we’ve fixed a good deal of bugs along the way with it.
It would be helpful to frame the discussion if there was a current list of issues that people are facing.
… On Nov 16, 2020, at 8:13 AM, jbrockmendel ***@***.***> wrote:
@xhochy <https://github.com/xhochy> has convinced me, largely based on the "new contributors will just be discouraged" point.
@WillAyd <https://github.com/WillAyd> could we remove -Werror and then check an env flag to potentially restore it? Then could suggest to experienced contributors that they should enable that flag.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#33315 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEU4UPM7DI3HI3IXLKOLCTSQFFT3ANCNFSM4MCESHFQ>.
|
|
If you're using the conda compilers on OS X, you'd see something like: conda-forge/pandas-feedstock#84 (comment) |
Because we already skip it on Windows as well ...
We can keep it o CI to catch regressions related to this.
The issue is actually because it inherently doesn't always match (locally you can have different OS version, compiler toolchain, etc), so that you run into different issues locally. It's impossible to make the two match exactly. Yes, it can be annoying that you locally don't get an error, while then CI gives errors. But at least that situation is clearer: CI is showing a compilation issue, that's something I need to solve / can ask help about on the PR (compared to always getting build errors when simply trying to build pandas locally before even starting to contribute). |
|
Ok sure let’s remove
…Sent from my iPhone
On Nov 17, 2020, at 12:07 AM, Joris Van den Bossche ***@***.***> wrote:
On most platforms there are no issues
Because we already skip it on Windows as well ...
and we’ve fixed a good deal of bugs along the way with it.
We can keep it o CI to catch regressions related to this.
we’ve been doing a lot of work elsewhere to make local development and CI match so would like to maintain that
The issue is actually because it inherently doesn't always match (locally you can have different OS version, compiler toolchain, etc), so that you run into different issues locally. It's impossible to make the two match exactly.
Yes, it can be annoying that you locally don't get an error, while then CI gives errors. But at least that situation is clearer: CI is showing a compilation issue, that's something I need to solve / can ask help about on the PR (compared to always getting build errors when simply trying to build pandas locally before even starting to contribute).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@xhochy would u push a PR to remove the flag but keep on CI; would do for 1.2 and 1.1.5 |
Already on it this minute! |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It seems nice to have
-Werrorin CI but as a conda-forge maintainer I quite often have to write patches to remove-Werrorfrom releases as the system distributions build packages on are not identical to the ones CI of upstream runs on. There is a large variety of compiler warnings and some may only show up when other C-defines are set. Also many new warning appear when you build with a newer compiler or different versions of the dependencies used.My suggestion would be:
-Werrorfromsetup.py.-WerrortoCFLAGSon CI to have the same result for pandas-dev as it is currently.Original PR: #32163
Related issues: #33314, #33224
The text was updated successfully, but these errors were encountered: