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

fix(gevent): unload time but not typing modules for module cloning #5135

Merged
merged 7 commits into from
Feb 16, 2023

Conversation

Yun-Kim
Copy link
Contributor

@Yun-Kim Yun-Kim commented Feb 15, 2023

This PR makes a fix to #4863 which inadvertently removed unloading the time module as well as removing the typing module from the list of standard library modules to not unload for module cloning. We found that the time module is a special case (which is important to unload due to interactions with gevent patching) as it is implicitly imported by recent versions of CPython on interpreter startup, so it may be present in sys.modules already.
The typing module is also problematic on being reloaded for older versions of Python < 3.7, so we should not unload/reload this module as well.

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Author is aware of the performance implications of this PR as reported in the benchmarks PR comment.

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer is aware of, and discussed the performance implications of this PR as reported in the benchmarks PR comment.

@Yun-Kim Yun-Kim added the changelog/no-changelog A changelog entry is not required for this PR. label Feb 15, 2023
@Yun-Kim Yun-Kim requested a review from a team as a code owner February 15, 2023 15:42
@Yun-Kim Yun-Kim requested review from majorgreys, mabdinur, emmettbutler and P403n1x87 and removed request for majorgreys and mabdinur February 15, 2023 15:42
@emmettbutler
Copy link
Collaborator

@Yun-Kim would you mind editing the riotfile to show which additional versions of python this creates support for?

@Yun-Kim Yun-Kim requested a review from a team as a code owner February 15, 2023 16:09
Copy link
Contributor

@P403n1x87 P403n1x87 left a comment

Choose a reason for hiding this comment

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

Some nits, otherwise LGTM.

ddtrace/bootstrap/sitecustomize.py Outdated Show resolved Hide resolved
riotfile.py Outdated Show resolved Hide resolved
@pr-commenter
Copy link

pr-commenter bot commented Feb 15, 2023

Benchmarks

Comparing candidate commit 2564674 in PR branch yunkim/module-cloning-typing-time with baseline commit 6ff460e in branch 1.x.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 56 cases.

@Yun-Kim
Copy link
Contributor Author

Yun-Kim commented Feb 15, 2023

@Yun-Kim would you mind editing the riotfile to show which additional versions of python this creates support for?

Looks like this change still doesn't support Python versions <= 3.7 (Hangs on 2.7, see memalloc issues on Python 3 up until 3.8+). The likely fix to support the older Python versions is contained in #5105.

@P403n1x87 P403n1x87 enabled auto-merge (squash) February 16, 2023 16:15
@P403n1x87 P403n1x87 merged commit bd20c55 into 1.x Feb 16, 2023
@P403n1x87 P403n1x87 deleted the yunkim/module-cloning-typing-time branch February 16, 2023 16:39
@github-actions github-actions bot added this to the v1.9.0 milestone Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants