GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1635741: Port _datetime extension module to multiphase initialization (PEP 489) by CharlieZhao95 · Pull Request #30522 · python/cpython

Conversation

@CharlieZhao95

@CharlieZhao95 CharlieZhao95 commented Jan 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Port _datetime extension module to multiphase initialization

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@CharlieZhao95
bpo-1635741:Port _datetime extension module to multiphase initializat…
5ee4157
…ion (PEP 489)

https://bugs.python.org/issue1635741
@CharlieZhao95
CharlieZhao95 requested review from abalkin and pganssle as code owners January 11, 2022 01:07
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Jan 11, 2022
@the-knights-who-say-ni

the-knights-who-say-ni commented Jan 11, 2022

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@CharlieZhao95

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting review label Jan 11, 2022
@blurb-it
📜🤖 Added by blurb_it.
0254dd9
@shihai1991

shihai1991 commented Jan 11, 2022

Copy link
Copy Markdown
Member

@CharlieZhao95 Thanks for your contirbute. You can use ./python -m test test_datetime -v -R 3:3 to catch the failure of CI.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 11, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  1. Please sign the CLA (See the devguide for more information).
  2. Most work related to bpo-1635741 has been put on hold by the SC (see SC communications as of Feb 2021). See also Victor's and my comment on bpo-1635741: port _ctypes extension module to multiphase initialization  #30525

Thank you for your interest in improving CPython! :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jan 12, 2022
@CharlieZhao95

CharlieZhao95 commented Jan 13, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author
  1. Please sign the CLA (See the devguide for more information).
  2. Most work related to bpo-1635741 has been put on hold by the SC (see SC communications as of Feb 2021). See also Victor's and my comment on [bpo-1635741: port _ctypes extension module to multiphase initialization  bpo-1635741: port _ctypes extension module to multiphase initialization  #30525](bpo-1635741: port _ctypes extension module to multiphase initialization  #30525)

Thank you for your interest in improving CPython! :)

Thanks for your reply :) Indeed, it is better to convert global state to module state first. I will work on issue https://bugs.python.org/issue40077 (convert static types to heap types) and fix reference leak promblem caused by static types in this PR later.

shihai1991 and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 13, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Thanks for your reply :) Indeed, it is better to convert global state to module state first. I will work on issue https://bugs.python.org/issue40077 (convert static types to heap types) and fix reference leak promblem caused by static types in this PR later.

Sounds good. But the steps needed should be approximately like this:

  1. Discuss the changes with the module maintainer/code owner (for example a topic on Discourse, or a bpo issue for this specific module). Explain the rationale and reasoning behind the change. Continue with the subsequent steps if the module maintainer approves.
  2. Lay out an implementation plan. Remember that there may be performance bottle necks when retrieving module state; care must be taken not to introduce a performance regression.
  3. Apply Argument Clinic to the modules types; it will help retrieving module state later in the process. Perhaps this is already done; I did not go to the gemba to look 🙂
  4. Establish a global module state
    • Create a module state struct and add a (temporary) instance on the module stack
    • Move global variables to module state struct one by one. (This is where issue 40077 happens!)
  5. Convert global module state to module state; take care that no performance regression is introduced => benchmark
  6. Apply multi-phase initialisation

IMO, this process should be spread out over several PRs, to make it easier to review. (I try to keep the diffs of my PRs below 200 lines of code.) It is IMO of importance that all of this happens in the course of a single alpha development phase. You cannot use a beta phase for changes like this. 3.11 beta starts in about 4 months time from now, IIRC.

I suggest you take a shot at it on your own fork, so you'll be ready for when the SC has approved we can continue with these changes 😃

shihai1991 and CharlieZhao95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

Oh, thanks for signing the CLA! 🚀 😃

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CharlieZhao95
CharlieZhao95 marked this pull request as draft January 18, 2022 07:02
@CharlieZhao95 CharlieZhao95 closed this Jan 21, 2022
@erlend-aasland

erlend-aasland commented Jun 27, 2022

Copy link
Copy Markdown
Contributor

FYI, PEP-687 was just accepted.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed
@CharlieZhao95 CharlieZhao95 mentioned this pull request Mar 30, 2023
Isolate Stdlib Extension Modules #103092
Closed
24 of 25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abalkin abalkin Awaiting requested review from abalkin
@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
@StanFromIreland StanFromIreland Awaiting requested review from StanFromIreland StanFromIreland will be requested when the pull request is marked ready for review StanFromIreland is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@CharlieZhao95 @the-knights-who-say-ni @shihai1991 @erlend-aasland @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.