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

[Python] test_total_bytes_allocated failing on CI #35728

Closed
h-vetinari opened this issue May 24, 2023 · 6 comments · Fixed by #36355
Closed

[Python] test_total_bytes_allocated failing on CI #35728

h-vetinari opened this issue May 24, 2023 · 6 comments · Fixed by #36355
Assignees
Milestone

Comments

@h-vetinari
Copy link
Contributor

Describe the bug, including details regarding any error messages, version, and platform.

In conda-forge/arrow-cpp-feedstock#1053, I currently have a failing test test_total_bytes_allocated on arrow 12 (though not arrow 11 or before), but some commit between arrow 12 and current main seems to have fixed this1 - any ideas?

This is currently holding up all our migrations on the arrow-feedstock and is very high priority for us in conda-forge. Possible solutions:

  • someone tells me that it's fine to skip that test
  • we identify the relevant changes that regressed this from 11->12 and revert them (on the feedstock)
  • we identify what fixed the issue between 12 & main and I backport those changes (on the feedstock)

Originally posted by @h-vetinari in #35637 (comment)

Component(s)

Packaging

Footnotes

  1. as confirmed in GH-35658: [Packaging] Sync conda recipes with feedstocks #35637

@assignUser
Copy link
Member

@westonpace is it fine to skip that test or do you know if that fix was already added for 12.0.1?

@h-vetinari
Copy link
Contributor Author

Thanks @assignUser for chiming in! Some more context. It did not appear for the last successful build of 12.0.0, then appeared for a migration (of simultaneously re2, aws-crt & google-cloud-cpp; because reasons...), but only on linux, and not for any of the older arrow versions (which received the same migrator PRs).

Single test failure here, and only on linux. Normally I'd go with a skip, but this looks like it might be leaking memory? Any ideas? The test in question has no comments or documentation AFAICS.

=================================== FAILURES ===================================
__________________________ test_total_bytes_allocated __________________________

    def test_total_bytes_allocated():
>       assert pa.total_allocated_bytes() == 0
E       assert 1216 == 0
E        +  where 1216 = <built-in function total_allocated_bytes>()
E        +    where <built-in function total_allocated_bytes> = pa.total_allocated_bytes

It's also interesting that this doesn't appear in any of the equivalent PRs to the maintenance branches, e.g. #1057.

However, I've now restarted conda-forge/arrow-cpp-feedstock#1053, and although I have no idea why, the failure seems to have disappeared! 🥳

I'm happy to close this issue (and apologise for the noise), unless people want to leave this open a bit longer to understand better what's happening with test_total_bytes_allocated. As I mentioned above, it's hard to tell for me (from lack of code comments/context) what this test is actually testing, under which conditions it may fail, and how serious that is if it happens.

@jorisvandenbossche
Copy link
Member

I think the context for that test is that in theory this method should always return zero if there is no data allocated/held by Arrow. And we are careful to ensure that in the tests we only create arrow data inside a test function (and for example not in the pytest.mark.parametrize values), to ensure that this assertion holds.

Now, why it would have temporarily failed, no idea.

@jorisvandenbossche jorisvandenbossche changed the title BUG: test_total_bytes_allocated failing on arrow 12 [Python] test_total_bytes_allocated failing on arrow 12 May 24, 2023
@pitrou
Copy link
Member

pitrou commented May 24, 2023

It might be that some data was held alive by the Python GC at this point, or perhaps some thread was lingering around? We should at least try to GC-collect at the start of the method.

@jorisvandenbossche jorisvandenbossche changed the title [Python] test_total_bytes_allocated failing on arrow 12 [Python] test_total_bytes_allocated failing on CI Jun 13, 2023
@jorisvandenbossche
Copy link
Member

This failure is not also happening on our own CI, for example the latest wheel-manylinux-2014-cp310-arm64 nightly build also has

 __________________________ test_total_bytes_allocated __________________________

    def test_total_bytes_allocated():
>       assert pa.total_allocated_bytes() == 0
E       assert 640 == 0
E        +  where 640 = <built-in function total_allocated_bytes>()
E        +    where <built-in function total_allocated_bytes> = pa.total_allocated_bytes

@jorisvandenbossche jorisvandenbossche added this to the 13.0.0 milestone Jun 13, 2023
@pitrou
Copy link
Member

pitrou commented Jun 14, 2023

Ok, perhaps we can simply execute this test in a subprocess to make it more reliable?

@raulcd raulcd added the Priority: Blocker Marks a blocker for the release label Jun 22, 2023
raulcd added a commit to raulcd/arrow that referenced this issue Jun 28, 2023
raulcd added a commit that referenced this issue Jun 29, 2023
…s to improve reliability (#36355)

### Rationale for this change

Currently some conda and wheels jobs are failing due to this error:

```
 =================================== FAILURES ===================================
__________________________ test_total_bytes_allocated __________________________

    def test_total_bytes_allocated():
>       assert pa.total_allocated_bytes() == 0
E       assert 1216 == 0
E        +  where 1216 = <built-in function total_allocated_bytes>()
E        +    where <built-in function total_allocated_bytes> = pa.total_allocated_bytes
```

### What changes are included in this PR?

As suggested on the issue trying to move this test to a subprocess to see if it improves reliabilit.

### Are these changes tested?

Archery and CI

### Are there any user-facing changes?

No
* Closes: #35728

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants