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

Collect after del in tests, add python 3.13 to non-dev CI #1835

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Sep 19, 2024

Description

This PR adds python 3.13 testing to our regular (non-devdeps) CI and adds gc.collect(2) calls after del calls in our tests.

I'm not certain the issue but my suspicion is that the python 3.13 changes to the garbage collector means that some of our tests that rely on implicit garbage collection may now intermittently fail with python 3.13. These tests often make an object, add it's reference in some way to asdf (which should grab a weakref for these tests), then del the object and check that asdf correctly handles the failure to resolve the weakref. If garbage collection doesn't occur between the del and the assert testing the weakref the test will fail. Hopefully adding explicit calls to gc.collect will fix the issue.

It looks like with python 3.13 gc.collect is equivalent to gc.collect(2). This is different compared to python 3.12 and the updates here use gc.collect(2) to try to have similar behavior for python 3.13 and older versions.

Closes #1834

Checklist:

  • pre-commit checks ran successfully

  • tests ran successfully

  • for a public change, added a towncrier news fragment

    changes/<PR#>.<changetype>.rst

    • changes/<PR#>.feature.rst: new feature
    • changes/<PR#>.bugfix.rst: bug fix
    • changes/<PR#>.doc.rst: documentation change
    • changes/<PR#>.removal.rst: deprecation or removal of public API
    • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • for a public change, updated documentation

  • for any new features, unit tests were added

@braingram braingram changed the title Collect after del Collect after del in tests, add python 3.13 to non-dev CI Sep 19, 2024
@braingram braingram marked this pull request as ready for review September 19, 2024 12:43
@braingram braingram requested a review from a team as a code owner September 19, 2024 12:43
Copy link
Member

@zacharyburnett zacharyburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea how the garbage collector works (do you just tell it to collect and it frees the memory?) but this LGTM

EDIT: oh I didn't read the description first, that makes sense

@braingram
Copy link
Contributor Author

Thanks for the review!

Here's a decent looking resource: https://stackify.com/python-garbage-collection/
and one that goes into low level details: https://devguide.python.org/internals/garbage-collector/index.html#garbage-collector-design

I opened a test PR to see if I could replicate the reported issue in our CI with no luck: #1836
I'm doing some local tests now (to try and reproduce the reported issue). If it's ok with you I'll leave this PR unmerged until I finish the local tests.

@braingram
Copy link
Contributor Author

Nice! I was able to replicate this with a dev install of python 3.13 (rc2+) and running the failing test in isolation. If I then add the gc.collect the test passes.

@braingram braingram merged commit 63ab7ad into asdf-format:main Sep 19, 2024
33 of 34 checks passed
@braingram braingram deleted the collect_after_del branch September 19, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.4.0] test_access_after_del fails with Python 3.13
2 participants