Error on C Warnings #32163
Error on C Warnings #32163
Conversation
|
on the py37 builds is this something we can handle on our end? |
| @@ -192,7 +192,7 @@ cdef class StringVector: | |||
|
|
|||
| append_data_string(self.data, x) | |||
|
|
|||
| cdef extend(self, ndarray[:] x): | |||
| cdef extend(self, ndarray[object] x): | |||
There was a problem hiding this comment.
im not sure this is accurate, might be a string dtype? IIRC this class isnt used
Potentially missing the PyDateTime_IMPORT macro which should initialise this. https://docs.python.org/3/c-api/datetime.html#datetime-objects |
|
Yea that and |
|
I think the issue here in internals might be a problem with the code Cython generates for generator functions. I opened cython/cython#3430 there so will see (@MomIsBestFriend I believe you were looking at that as well) |
| @@ -67,7 +67,7 @@ npy_datetime NpyDateTimeToEpoch(npy_datetime dt, NPY_DATETIMEUNIT base) { | |||
| } | |||
|
|
|||
| /* Convert PyDatetime To ISO C-string. mutates len */ | |||
| char *PyDateTimeToIso(PyDateTime_Date *obj, NPY_DATETIMEUNIT base, | |||
| char *PyDateTimeToIso(PyObject *obj, NPY_DATETIMEUNIT base, | |||
There was a problem hiding this comment.
Apparently PyDateTime_Date is not considered part of the public API and attempts have been made in the past to privatize:
python/cpython#10238 (comment)
So PyObject seems the right way to go here and gets rid of warnings
| if n == 0: | ||
| return | ||
|
|
||
| result = list() |
There was a problem hiding this comment.
I think there are some "bugs" in Cython with generator functions that were yielding build warnings. I opened an issue on the tracker for it here:
But for now figure OK to just convert to a list instead of a generator func anyway
|
OK this should be ready for review. w.r.t. the internals change I ran benchmarks for concat and reshape again and got the following: before after ratio
[80078ac0] [5fc220c0]
<master> <no-warnings>
- 3.81±0.2ms 3.18±0.02ms 0.83 reshape.Explode.time_explode(1000, 5)
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.(which I don't think is related) |
There was a problem hiding this comment.
lgtm. @jbrockmendel
how are the error messages if something is warning? intelligable?
|
Maybe misreading your question but compilation warnings will now show as error messages, so CI will fail
* note that Cython warnings are separate from compilation warnings, so those wont fail CI
…Sent from my iPad
On Mar 19, 2020, at 2:13 PM, Jeff Reback ***@***.***> wrote:
@jreback approved this pull request.
lgtm. @jbrockmendel
how are the error messages if something is warning? intelligable?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
ok great. |
|
thanks |
Using Clang and latest numpy locally looks like we can use
-Werror. WIP as I think there may still be things with gcc and older numpy versions to sort outNote that there is also a warning emitted by cython for unreachable code in groupby, but -Werror wouldn't affect the "cythonization" step.