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

CI: Troubleshoot CI #44822

Merged
merged 9 commits into from Dec 11, 2021
Merged

CI: Troubleshoot CI #44822

merged 9 commits into from Dec 11, 2021

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

Tacked some CI-troubleshooting stuff onto the just-started FIXMES21 branch.

@jreback jreback added the CI Continuous Integration label Dec 8, 2021
@jreback
Copy link
Contributor

jreback commented Dec 8, 2021

great lmk when ready

@jbrockmendel
Copy link
Member Author

Looks like npdev changed ndarray.mean s.t. np.arange(10).view('m8[ns]').mean() returns a float instead of a td64 obj cc @seberg

@seberg
Copy link
Contributor

seberg commented Dec 8, 2021

Wow, yet another weird case :(. Our mean code dares to force cast to "f8" for timedeltas, so we effectively call:

arr.sum(dtype="f8")

but, happy coincidence apparently the old datetime resolution is broken and just ignores the dtype request... Although, I am still surprised why it doesn't just fail in the new code.

Well, I guess, mean is arguably broken, if I get luck reverting numpy/numpy#20544 (comment) will "fix" this too (i.e. revert to the old borked up state).

@jreback jreback added this to the 1.4 milestone Dec 8, 2021
@jreback
Copy link
Contributor

jreback commented Dec 9, 2021

@jbrockmendel if you can fix this up

@jbrockmendel
Copy link
Member Author

waiting for a fix in npdev

@seberg
Copy link
Contributor

seberg commented Dec 9, 2021

Should be fixed, not sure if the nightly is build though.

@jreback
Copy link
Contributor

jreback commented Dec 9, 2021

waiting for a fix in npdev

ok if not fixed, then can you disable tests?

@jbrockmendel
Copy link
Member Author

will re-push and find out

@jbrockmendel
Copy link
Member Author

Looks like the td64.mean may have gotten fixed but not we're seeing a bunch of new warnings (guessing division by zero related). will dig into this in a bit

@seberg
Copy link
Contributor

seberg commented Dec 9, 2021

:(, sorry about this, its been a confusing can of worms. One thing that could still be changed that if you provide out= to a reduction, you may end up with a more precise loop (which I suppose could mean float if the array is an integer, but the out is a float).

But I guess this is something awfully gone wrong, or just a different error?:

numpy.core._exceptions._UFuncBinaryResolutionError: ufunc 'divide' cannot use operands with types dtype('int8') and dtype('<m8[ns]')

Where apparently the type resolution is somehow failing (which confuses me a lot, because I am not sure why this would have been any better with the previous version.)

@jbrockmendel
Copy link
Member Author

But I guess this is something awfully gone wrong, or just a different error?:

Yah in these cases we're currently testing that we get a TypeError, so getting a numpy.core._exceptions._UFuncBinaryResolutionError instead isn't the worst thing in the world; if it can't/won't be changed upstream then we can try to catch and re-raise

@seberg
Copy link
Contributor

seberg commented Dec 9, 2021

Ah OK, puh, I was seriously worried it was not an error before... That error should be inheriting from TypeError, so I think I would prefer to just ignore it, at least from the NumPy side. But please complain if you think that is annoying.

@jbrockmendel
Copy link
Member Author

I think the exception is fine. The failing tests look like a new warning which im assuming is division by zero, which is also pretty reasonable so we can adjust to handle on our end.

@seberg
Copy link
Contributor

seberg commented Dec 9, 2021

Cool, thanks. Let me know if something is looking shady there, so I can have a closer look.

@jreback
Copy link
Contributor

jreback commented Dec 9, 2021

@jbrockmendel if you have a chance to fix up here (i don't care if you xfail these tests) but cannot merge anything else until we are back to green.

@jbrockmendel
Copy link
Member Author

its gonna take a couple more rounds on the CI bc i cant get npdev working locally

@jbrockmendel
Copy link
Member Author

jbrockmendel commented Dec 10, 2021

It looks very tentatively like a lot of np_obj.__op__(pd_obj) calls are going through pd_obj__array_ufunc__ instead of returning NotImplemented, which we want so they can use pd_obj.__rop__(np_obj)

@jreback
Copy link
Contributor

jreback commented Dec 10, 2021

maybe worthile to simply fix the numpy dev wheel to a working one while this is fixed?

@jbrockmendel
Copy link
Member Author

maybe worthile to simply fix the numpy dev wheel to a working one while this is fixed?

first should confirm with @seberg whether the guess about __array_ufunc__ is correct, and if so if it is something they intend to fix

@jreback
Copy link
Contributor

jreback commented Dec 10, 2021

sorry what i mean is pin the wheel to a working one from 2 days ago

@jbrockmendel
Copy link
Member Author

i guess. no idea how to do that

@lithomas1
Copy link
Member

numpy==1.23.0.dev0+101.ga81535a36 in the yaml files?

@jbrockmendel
Copy link
Member Author

@lithomas1 can you pinch-hit on this one?

@lithomas1
Copy link
Member

Sure, #44834 is my attempt. We should still keep this PR open to revert the pin in there and come up with a long-term fix.

@jbrockmendel
Copy link
Member Author

sounds good. ive confirmed locally that the td64 object is not respecting __array_priority__ on numpy:main

@seberg
Copy link
Contributor

seberg commented Dec 10, 2021

@jbrockmendel sorry, only catching up now/tomorrow. I cannot think of immediate changes related to __array_priority__ or binary op's behaviour. This is for numpy's timedelta64 scalars?

@seberg
Copy link
Contributor

seberg commented Dec 10, 2021

(I guess one thing I am confused, is because if __array_ufunc__ is defined, I believe the design is that the binary op's go through __array_ufunc__ and never return NotImplemented, unless __array_ufunc__ is None?)

@seberg
Copy link
Contributor

seberg commented Dec 10, 2021

Ah, I had looked at the wrong CI run yesterday, I think. But I am still confused what the issue is? Do you have an example?

Btw. assuming you are on linux, one quick hack that may work to get numpy dev, is:

python3 runtests.py --shell

to drop in a shell with numpy dev version injected into the PYTHONPATH.

@jbrockmendel
Copy link
Member Author

I got npdev working locally by installing from source instead of using the wheel. So I can confirm that a) the timedelta64.__truediv__ method is not returning NotImplemented as I think it should and b) that actually isn't new (numpy/numpy#17017), so isn't the explanation here.

@jbrockmendel
Copy link
Member Author

Found a likely candidate for a bug in our __array_ufunc__ implementation. just pushed.

@jbrockmendel
Copy link
Member Author

Numpy pin reverted + green

@jreback
Copy link
Contributor

jreback commented Dec 11, 2021

great

going to merge but is there anything we should backport to 1.3.5?

@jbrockmendel
Copy link
Member Author

if we want 1.3.5 to work with npdev then the one line in ops_dispatch.pyx would need to be backported i think

@jreback
Copy link
Contributor

jreback commented Dec 11, 2021

ok let's try to backport this and then

@jreback jreback modified the milestones: 1.4, 1.3.5 Dec 11, 2021
@jreback jreback merged commit f08f574 into pandas-dev:master Dec 11, 2021
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Dec 11, 2021
@jreback
Copy link
Contributor

jreback commented Dec 11, 2021

@meeseeksdev backport 1.3.x

@lumberbot-app
Copy link

lumberbot-app bot commented Dec 11, 2021

Something went wrong ... Please have a look at my logs.

jreback pushed a commit that referenced this pull request Dec 11, 2021
@jbrockmendel jbrockmendel deleted the fixmes21 branch December 11, 2021 18:03
HyukjinKwon pushed a commit to apache/spark that referenced this pull request Jul 5, 2022
### What changes were proposed in this pull request?
This PR fix the wrong aliases in `__array_ufunc__`

### Why are the changes needed?
When running test with numpy 1.23.0 (current latest), hit a bug: `NotImplementedError: pandas-on-Spark objects currently do not support <ufunc 'divide'>.`

In `__array_ufunc__` we first call `maybe_dispatch_ufunc_to_dunder_op` to try dunder methods first, and then we try pyspark API. `maybe_dispatch_ufunc_to_dunder_op` is from pandas code.

pandas fix a bug pandas-dev/pandas#44822 (comment) pandas-dev/pandas@206b249 when upgrade to numpy 1.23.0, we need to also sync this.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Current CI passed
- The exsiting UT `test_series_datetime` already cover this, I also test it in my local env with 1.23.0
```shell
pip install "numpy==1.23.0"
python/run-tests --testnames 'pyspark.pandas.tests.test_series_datetime SeriesDateTimeTest.test_arithmetic_op_exceptions'
```

Closes #37078 from Yikun/SPARK-39611.

Authored-by: Yikun Jiang <yikunkero@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
HyukjinKwon pushed a commit to apache/spark that referenced this pull request Jul 5, 2022
### What changes were proposed in this pull request?
This PR fix the wrong aliases in `__array_ufunc__`

### Why are the changes needed?
When running test with numpy 1.23.0 (current latest), hit a bug: `NotImplementedError: pandas-on-Spark objects currently do not support <ufunc 'divide'>.`

In `__array_ufunc__` we first call `maybe_dispatch_ufunc_to_dunder_op` to try dunder methods first, and then we try pyspark API. `maybe_dispatch_ufunc_to_dunder_op` is from pandas code.

pandas fix a bug pandas-dev/pandas#44822 (comment) pandas-dev/pandas@206b249 when upgrade to numpy 1.23.0, we need to also sync this.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Current CI passed
- The exsiting UT `test_series_datetime` already cover this, I also test it in my local env with 1.23.0
```shell
pip install "numpy==1.23.0"
python/run-tests --testnames 'pyspark.pandas.tests.test_series_datetime SeriesDateTimeTest.test_arithmetic_op_exceptions'
```

Closes #37078 from Yikun/SPARK-39611.

Authored-by: Yikun Jiang <yikunkero@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit fb48a14)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
HyukjinKwon pushed a commit to apache/spark that referenced this pull request Jul 5, 2022
### What changes were proposed in this pull request?
This PR fix the wrong aliases in `__array_ufunc__`

### Why are the changes needed?
When running test with numpy 1.23.0 (current latest), hit a bug: `NotImplementedError: pandas-on-Spark objects currently do not support <ufunc 'divide'>.`

In `__array_ufunc__` we first call `maybe_dispatch_ufunc_to_dunder_op` to try dunder methods first, and then we try pyspark API. `maybe_dispatch_ufunc_to_dunder_op` is from pandas code.

pandas fix a bug pandas-dev/pandas#44822 (comment) pandas-dev/pandas@206b249 when upgrade to numpy 1.23.0, we need to also sync this.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Current CI passed
- The exsiting UT `test_series_datetime` already cover this, I also test it in my local env with 1.23.0
```shell
pip install "numpy==1.23.0"
python/run-tests --testnames 'pyspark.pandas.tests.test_series_datetime SeriesDateTimeTest.test_arithmetic_op_exceptions'
```

Closes #37078 from Yikun/SPARK-39611.

Authored-by: Yikun Jiang <yikunkero@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit fb48a14)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
sunchao pushed a commit to sunchao/spark that referenced this pull request Jun 2, 2023
### What changes were proposed in this pull request?
This PR fix the wrong aliases in `__array_ufunc__`

### Why are the changes needed?
When running test with numpy 1.23.0 (current latest), hit a bug: `NotImplementedError: pandas-on-Spark objects currently do not support <ufunc 'divide'>.`

In `__array_ufunc__` we first call `maybe_dispatch_ufunc_to_dunder_op` to try dunder methods first, and then we try pyspark API. `maybe_dispatch_ufunc_to_dunder_op` is from pandas code.

pandas fix a bug pandas-dev/pandas#44822 (comment) pandas-dev/pandas@206b249 when upgrade to numpy 1.23.0, we need to also sync this.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Current CI passed
- The exsiting UT `test_series_datetime` already cover this, I also test it in my local env with 1.23.0
```shell
pip install "numpy==1.23.0"
python/run-tests --testnames 'pyspark.pandas.tests.test_series_datetime SeriesDateTimeTest.test_arithmetic_op_exceptions'
```

Closes apache#37078 from Yikun/SPARK-39611.

Authored-by: Yikun Jiang <yikunkero@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit fb48a14)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants