enable ASAN/UBSAN in pandas CI
State: closed

@lithomas1 @rgommers 

closes https://github.com/pandas-dev/pandas/issues/52990

Labels:
name: Build
description: Library building on various platforms

Comments:
Somewhat working now.  Guessing we need a way for pytest-xdist to fail and signal the test it failed on. Right now looks like a worker just crashed
This looks pretty good to me. Let me Cc @ngoldbaum, who implemented the NumPy CI job and has more experience with these sanitizers than I have.
> Guessing we need a way for pytest-xdist to fail and signal the test it failed on.

Ah that's probably why the `halt_on_error=1` didn't work, I wasn't running with `pytest-xdist` locally when I was setting up the numpy config.
> Somewhat working now. Guessing we need a way for pytest-xdist to fail and signal the test it failed on. Right now looks like a worker just crashed

IMO I would add 1 new testing job in the matrix (e.g. that uses a 3.11 dependency file) that runs the tests with sanitize=True and python xdist with `-n 0`
In that case are you still planning to run against the entire test base or a subset of modules? I think removing multiple workers would slow down our CI a good deal? But maybe this gets to a state where it only runs when C/Cython files are touched?
Worth noting I tried `-n 0` locally a few times and it didn't make a difference. Not sure if the mere installation of pytest-xdist changes that. Needs further investigation
> In that case are you still planning to run against the entire test base or a subset of modules? I think removing multiple workers would slow down our CI a good deal? But maybe this gets to a state where it only runs when C/Cython files are touched?

We should do a minimal run, kind of like the npdev situation. So only us, numpy, and arrow installed.
Maybe that helps with the runtime?
Since the GHA Ubuntu runners only have 2 cores, I think running the entire test suite (even with all the dependencies) with `-n 0` will be _that_ significant, thought I don't know the impact that will have on the debugger.

FWIW that's how the Windows tests run currently and they take 15ish minutes longer than the non xdist runs
> thought I don't know the impact that will have on the debugger.

In theory the average runtime of ASAN would be 2x (see https://github.com/google/sanitizers/wiki/AddressSanitizer), though since we are not detecting leaks but also adding UBSAN I'm not sure how that all evens out
A lot of the datetime stuff in this PR is hacked together just to appease UBSAN, but there are definitely quite a few code paths where datetime conversions can lead to undefined behavior. 

The current ASAN failure looks like it comes from matplotlib, so @lithomas1 is probably right in that we need to pare this down to a smaller set of packages that we know can be clean
This pull request is stale because it has been open for thirty days with no activity. Please [update](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request) and respond to this comment if you're still interested in working on this.
Not entirely sure what the current CI issue is - @lithomas1 any chance you've seen this before?

ERROR: MISSING test execution (tx) nodes: please specify --tx
Ok I think this is ready in its current form. Some things that can be improved in follow ups:

- more coverage. Runtime is good, but the full suite was timing out. A better balance can be found
- cleaning up some of the skipped known_ub; I think these are all integer overflows
- Improving CI error messaging. Right now an error causes CI to fail, but the messaging back from a failed node through pytest-xdist needs improvement 
Any feedback on this? I think would be nice to get in before the next release. Hope this eliminates the segfaults popping up during release
I'll look in a couple more days, if you still need my review.

Just finished finals, so gonna take a breather for a bit before doing pandas stuff.
Ah not yet unfortunately. The scope of this as is detects undefined behavior and address violations. ASAN can normally check memory leaks for you, but we had to add `detect_leaks=0` as an argument to the environment because the Python runtime seems to show all types of leaks

Whether those are legitimate or not requires more investigation. We could try to include a leak suppression file as described here https://github.com/pandas-dev/pandas/issues/54865#issuecomment-1839955886 but some of those suppressions also cast a very wide net to the point that it may not be all that useful
Gotcha. Or rather, if we introduce undefined behavior and address violations, this job will hopefully fail correct? Just want to ensure there's a definitive `job failure -> rectification -> job success` path for this job
Yes exactly - this will fail when either of those are detected
The error messaging you see in CI is something that could be improved. It just "fails" right now but that feedback gets lost along the way from the crashed process. I think that can be tackled in a follow up
This is what happens today if either of these pops up:

https://github.com/pandas-dev/pandas/actions/runs/7066657149/job/19241456914#step:8:61
> This is what happens today if either of these pops up:
> 
> https://github.com/pandas-dev/pandas/actions/runs/7066657149/job/19241456914#step:8:61

Ah OK. Could you at least include a `test_args: "-v"` in the job configuration in `unit-test.yml`? At least then the last test should be printed before the job fails so when it fails we don't have to rerun to figure out where this fails
OK sure. Here is what that looks like:

https://github.com/pandas-dev/pandas/actions/runs/7267349987/job/19800967532?pr=55102#step:8:18053

Ends up being too much for GHA to show but if you go to the raw logs you will see the error:

<details>

```
2023-12-19T21:11:17.6390164Z ../../pandas/_libs/src/vendored/ujson/python/objToJSON.c:2066:3: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
2023-12-19T21:12:44.8157270Z pandas/tests/io/test_common.py::TestCommonIOCapabilities::test_write_missing_parent_directory[to_json-os-OSError-json] 
2023-12-19T21:12:44.8158122Z [gw3] node down: Not properly terminated
2023-12-19T21:12:44.8158390Z 
2023-12-19T21:12:44.8158505Z replacing crashed worker gw3
2023-12-19T21:12:44.8158882Z INTERNALERROR> Traceback (most recent call last):
2023-12-19T21:12:44.8159868Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/_pytest/main.py", line 271, in wrap_session
2023-12-19T21:12:44.8160796Z INTERNALERROR>     session.exitstatus = doit(config, session) or 0
2023-12-19T21:12:44.8161413Z INTERNALERROR>                          ^^^^^^^^^^^^^^^^^^^^^
2023-12-19T21:12:44.8162455Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/_pytest/main.py", line 325, in _main
2023-12-19T21:12:44.8163512Z INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
2023-12-19T21:12:44.8164538Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/pluggy/_hooks.py", line 493, in __call__
2023-12-19T21:12:44.8165536Z INTERNALERROR>     return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
2023-12-19T21:12:44.8166476Z INTERNALERROR>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2023-12-19T21:12:44.8167481Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/pluggy/_manager.py", line 115, in _hookexec
2023-12-19T21:12:44.8168482Z INTERNALERROR>     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2023-12-19T21:12:44.8169138Z INTERNALERROR>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2023-12-19T21:12:44.8170131Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/pluggy/_callers.py", line 152, in _multicall
2023-12-19T21:12:44.8170977Z INTERNALERROR>     return outcome.get_result()
2023-12-19T21:12:44.8171378Z INTERNALERROR>            ^^^^^^^^^^^^^^^^^^^^
2023-12-19T21:12:44.8172287Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/pluggy/_result.py", line 114, in get_result
2023-12-19T21:12:44.8173179Z INTERNALERROR>     raise exc.with_traceback(exc.__traceback__)
2023-12-19T21:12:44.8174405Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/pluggy/_callers.py", line 77, in _multicall
2023-12-19T21:12:44.8175220Z INTERNALERROR>     res = hook_impl.function(*args)
2023-12-19T21:12:44.8175633Z INTERNALERROR>           ^^^^^^^^^^^^^^^^^^^^^^^^^
2023-12-19T21:12:44.8176555Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/xdist/dsession.py", line 123, in pytest_runtestloop
2023-12-19T21:12:44.8177342Z INTERNALERROR>     self.loop_once()
2023-12-19T21:12:44.8178296Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/xdist/dsession.py", line 148, in loop_once
2023-12-19T21:12:44.8179020Z INTERNALERROR>     call(**kwargs)
2023-12-19T21:12:44.8179889Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/xdist/dsession.py", line 273, in worker_collectionfinish
2023-12-19T21:12:44.8180703Z INTERNALERROR>     self.sched.schedule()
2023-12-19T21:12:44.8181588Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/xdist/scheduler/loadscope.py", line 339, in schedule
2023-12-19T21:12:44.8182381Z INTERNALERROR>     self._reschedule(node)
2023-12-19T21:12:44.8183278Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/xdist/scheduler/loadscope.py", line 321, in _reschedule
2023-12-19T21:12:44.8184099Z INTERNALERROR>     self._assign_work_unit(node)
2023-12-19T21:12:44.8185057Z INTERNALERROR>   File "/home/runner/micromamba/envs/test/lib/python3.11/site-packages/xdist/scheduler/loadscope.py", line 259, in _assign_work_unit
2023-12-19T21:12:44.8186005Z INTERNALERROR>     worker_collection = self.registered_collections[node]
2023-12-19T21:12:44.8186635Z INTERNALERROR>                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
2023-12-19T21:12:44.8187150Z INTERNALERROR> KeyError: <WorkerController gw7>
```

</details>
Great thanks!
> > This is what happens today if either of these pops up:
> > https://github.com/pandas-dev/pandas/actions/runs/7066657149/job/19241456914#step:8:61
> 
> Ah OK. Could you at least include a `test_args: "-v"` in the job configuration in `unit-test.yml`? At least then the last test should be printed before the job fails so when it fails we don't have to rerun to figure out where this fails

Hm, I wonder if there's a better way to do this.
(My main worry is that this make the logs very hard to scroll through, e.g. if I had to look through the logs because of a flaky non-ASAN related test)

Do you know if e.g. something like ``PYTEST_CURRENT_TEST`` might help?
https://docs.pytest.org/en/7.1.x/example/simple.html#pytest-current-test-env
Is there anything particular to CI that we know of that does not redirect stderr from pytest-xdist to the logs? If you run things locally you get the error and a stacktrace. While that isn't 1-to-1 to the test name it is pretty helpful to figure out what is going on so might be the best medium
> Is there anything particular to CI that we know of that does not redirect stderr from pytest-xdist to the logs? 

If I understand your question, this is a pytest xdist limitation: https://pytest-xdist.readthedocs.io/en/stable/known-limitations.html#output-stdout-and-stderr-from-workers

I know the `-v` isn't elegant, but I would prefer to have _a_ way to narrow down what is causing the failure before merging this in.
> I know the `-v` isn't elegant, but I would prefer to have _a_ way to narrow down what is causing the failure before merging this in.

I agree. I don't mean to block this PR, but maybe we should look into the reporting a little more?

At the very least, I think I could settle for a solution where we get pytest to print the filenames of the tests.
We could have this if we turned off pytest-xdist.
(there seems to be an issue with pytest-xdist where it swallows the filenames.
https://github.com/pytest-dev/pytest-xdist/issues/450)

Would this work?
OK here is what @lithomas1 suggestion looks like:

https://github.com/pandas-dev/pandas/actions/runs/7281808581/job/19843041899?pr=55102#step:8:590

Out of the two options so far I would prefer to go that route. It looks like turning off pytest-xdist for the ASAN build had little to no effect on the overall runtime
> OK here is what @lithomas1 suggestion looks like:
> 
> https://github.com/pandas-dev/pandas/actions/runs/7281808581/job/19843041899?pr=55102#step:8:590
> 
> Out of the two options so far I would prefer to go that route. It looks like turning off pytest-xdist for the ASAN build had little to no effect on the overall runtime

Ok, this looks correct to me, at a first glance. Just to double check, the failing test happens in ``test_common.py``, right?
(and not in ``pandas/tests/extension/json/test_json.py``)
Yea it does happen in test_common. I suppose the downside to this one is you don't get exactly the test that failed, but the first one I see failing locally is `pandas/tests/io/test_common.py::TestCommonIOCapabilities::test_write_missing_parent_directory[to_json-os-OSError-json]`