FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-101819: Isolate _io by erlend-aasland · Pull Request #101948 · python/cpython · GitHub

/ cpython Public

gh-101819: Isolate _io - #101948

Merged
kumaraditya303 merged 86 commits into
python:mainfrom
erlend-aasland:isolate-io/poc
May 15, 2023
ConversationCommits (86)ChecksFiles changed
Merged

gh-101819: Isolate _io#101948
kumaraditya303 merged 86 commits into
python:mainfrom
erlend-aasland:isolate-io/poc

Conversation

erlend-aasland commented Feb 16, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Proof-of-concept. We will split this change up in multiple PRs

bedevere-bot mentioned this pull request Feb 16, 2023
Closed
kumaraditya303 previously requested changes Feb 20, 2023
View reviewed changes
Comment thread Modules/_io/iobase.c Outdated

bedevere-bot commented Feb 20, 2023

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

bedevere-bot commented May 12, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit ab1baf4 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 12, 2023

erlend-aasland commented May 12, 2023

Copy link
Copy Markdown
Contributor Author

I am not sure how it is not needed. I see similar thing done in _csv module.

Hm, Serhiy's general fix (gh-22870) was committed just before the _csv extension module was adapted (gh-23224).

When I try pickling with protocols 0 and 1 in the REPL, I get a correct TypeError for the _io types. It's strange that this is not the behaviour we're also seeing in test_pickling.

kumaraditya303 commented May 13, 2023

Copy link
Copy Markdown
Contributor

When I try pickling with protocols 0 and 1 in the REPL, I get a correct TypeError for the _io types. It's strange that this is not the behaviour we're also seeing in test_pickling.

Did you remove the _PyIOBase_cannot_pickle before doing that? I tried and it pickled without error. Anyways I am not really an expert on pickle.

erlend-aasland commented May 14, 2023

Copy link
Copy Markdown
Contributor Author

Did you remove the _PyIOBase_cannot_pickle before doing that? I tried and it pickled without error. Anyways I am not really an expert on pickle.

Probably not; I tried again yesterday, and it also pickled without error. I'm not an expert on pickle either, but it seems to me we've found a corner case where Serhiy's fix of 2020 does not apply. One option can be to fix Lib/copyreg.py. In any case, I suggest to land this PR with the pickle workaround in it. If we end up patching Serhiy's general pickle fix, we can revert the workaround introduced here (as Serhiy's patch did with other extension modules back in 2020).

kumaraditya303 commented May 15, 2023

Copy link
Copy Markdown
Contributor

Agreed, I'll make a final review and merge by later today.

kumaraditya303 commented May 15, 2023

Copy link
Copy Markdown
Contributor 
root@codespaces-62bef3 /w/cpython (isolate-io/poc) [SIGINT]# ./python -m test -R 3:3 test_io
0:00:00 load avg: 7.01 Run tests sequentially
0:00:00 load avg: 7.01 [1/1] test_io
beginning 6 repetitions
123456
......
test_io passed in 3 min 31 sec

== Tests result: SUCCESS ==

1 test OK.

Total duration: 3 min 31 sec
Tests result: SUCCESS

kumaraditya303 enabled auto-merge (squash) May 15, 2023 09:17
kumaraditya303 merged commit 186bf39 into python:main May 15, 2023
kumaraditya303 deleted the isolate-io/poc branch May 15, 2023 09:26

erlend-aasland commented May 15, 2023

Copy link
Copy Markdown
Contributor Author

Finally!

carljm added a commit to carljm/cpython that referenced this pull request May 15, 2023
Merge branch 'main' into supermon
343867a 
* main: (29 commits)
  pythongh-101819: Fix _io clinic input for unused base class method stubs (python#104418)
  pythongh-101819: Isolate `_io` (python#101948)
  Bump mypy from 1.2.0 to 1.3.0 in /Tools/clinic (python#104501)
  pythongh-104494: Update certain Tkinter pack/place tests for Tk 8.7 errors (python#104495)
  pythongh-104050: Run mypy on `clinic.py` in CI (python#104421)
  pythongh-104490: Consistently define phony make targets (python#104491)
  pythongh-67056: document that registering/unregistering an atexit func from within an atexit func is undefined (python#104473)
  pythongh-104487: PYTHON_FOR_REGEN must be minimum Python 3.10 (python#104488)
  pythongh-101282: move BOLT config after PGO (pythongh-104493)
  pythongh-104469 Convert _testcapi/float.c to use AC (pythongh-104470)
  pythongh-104456: Fix ref leak in _ctypes.COMError (python#104457)
  pythongh-98539: Make _SSLTransportProtocol.abort() safe to call when closed (python#104474)
  pythongh-104337: Clarify random.gammavariate doc entry  (python#104410)
  Minor improvements to typing docs (python#104465)
  pythongh-87092: avoid gcc warning on uninitialized struct field in assemble.c (python#104460)
  pythonGH-71383: IDLE - Document testing subsets of modules (python#104463)
  pythongh-104454: Fix refleak in AttributeError_reduce (python#104455)
  pythongh-75710: IDLE - add docstrings and comments to editor module (python#104446)
  pythongh-91896: Revert some very noisy DeprecationWarnings for `ByteString` (python#104424)
  Add a mention of PYTHONBREAKPOINT to breakpoint() docs (python#104430)
  ...
carljm added a commit to carljm/cpython that referenced this pull request May 15, 2023
Merge branch 'main' into cached_property_docs
452d9f5 
* main: (204 commits)
  pythongh-101819: Fix _io clinic input for unused base class method stubs (python#104418)
  pythongh-101819: Isolate `_io` (python#101948)
  Bump mypy from 1.2.0 to 1.3.0 in /Tools/clinic (python#104501)
  pythongh-104494: Update certain Tkinter pack/place tests for Tk 8.7 errors (python#104495)
  pythongh-104050: Run mypy on `clinic.py` in CI (python#104421)
  pythongh-104490: Consistently define phony make targets (python#104491)
  pythongh-67056: document that registering/unregistering an atexit func from within an atexit func is undefined (python#104473)
  pythongh-104487: PYTHON_FOR_REGEN must be minimum Python 3.10 (python#104488)
  pythongh-101282: move BOLT config after PGO (pythongh-104493)
  pythongh-104469 Convert _testcapi/float.c to use AC (pythongh-104470)
  pythongh-104456: Fix ref leak in _ctypes.COMError (python#104457)
  pythongh-98539: Make _SSLTransportProtocol.abort() safe to call when closed (python#104474)
  pythongh-104337: Clarify random.gammavariate doc entry  (python#104410)
  Minor improvements to typing docs (python#104465)
  pythongh-87092: avoid gcc warning on uninitialized struct field in assemble.c (python#104460)
  pythonGH-71383: IDLE - Document testing subsets of modules (python#104463)
  pythongh-104454: Fix refleak in AttributeError_reduce (python#104455)
  pythongh-75710: IDLE - add docstrings and comments to editor module (python#104446)
  pythongh-91896: Revert some very noisy DeprecationWarnings for `ByteString` (python#104424)
  Add a mention of PYTHONBREAKPOINT to breakpoint() docs (python#104430)
  ...

vstinner commented May 15, 2023

Copy link
Copy Markdown
Member

It's good to see _PyIO_get_module_state() going away :-) Also, bye bye my static_types and _PyIO_FiniTypes() workarounds!

I'm happy that these changes were merged as a long list of changes (see PR list in issue #101819): if something will go wrong, it will be easier to identify which sub-part of these changes is causing troubles. I expect troubles, but that's fine. We already have to go trough turbulences to modernize Python ;-)

Thanks @erlend-aasland and @kumaraditya303 for your hard work on this complicated _io extension.

sunmy2019 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 15, 2023

bedevere-bot commented May 15, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @sunmy2019 for commit fe2db1b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 15, 2023

erlend-aasland commented May 15, 2023

Copy link
Copy Markdown
Contributor Author

@sunmy2019, why did you engage the refleak bots on a closed PR? Did you check the results of the refleak run that Kumar did for the same commit (fe2db1b) yesterday first? (All 31 checks passed.)

sunmy2019 commented May 16, 2023

Copy link
Copy Markdown
Member

It should not pass the ref leaks tests. #104510 Just rerun to check what's going on.

Also, I should not trigger builtbot on a merged PR. It seems nothing will be tested. That's my bad.

erlend-aasland commented May 16, 2023

Copy link
Copy Markdown
Contributor Author

Also, I should not trigger builtbot on a merged PR. It seems nothing will be tested. That's my bad.

Yes. And you can examine the refleak run Kumar did post merge. I did yesterday. I'm not sure why that passed, though.

vstinner commented May 17, 2023

Copy link
Copy Markdown
Member

Yes. And you can examine the refleak run Kumar did post merge. I did yesterday. I'm not sure why that passed, though.

Your PR was created in February. Maybe the code evolved in the meanwhile and the final "squash + rebase" commit is different than the PR branch ran on buildbots. For such PR which is in the works for a long time, I prefer to manually squash+rebase time to time to workaround this workflow limitation. There are services like https://mergify.com/ which prevent this workflow flaw. I heard that GitHub automerge can do something similar (run GHA jobs on the "final" commit), but buildbots are not handled and use the old way (run the jobs on the PR branch which is not merged into main / rebased).

musicinmybrain added a commit to musicinmybrain/indexed_gzip that referenced this pull request Jul 11, 2023
Replace IndexedGzipFile.__reduce__ with IndexedGzipFile.__reduce_ex__
0b8807e 
Restores “pickle-ability” of IndexedGzipFile in Python 3.12, which was
broken due to python/cpython#101948.
musicinmybrain mentioned this pull request Jul 11, 2023
Merged
musicinmybrain added a commit to musicinmybrain/indexed_gzip that referenced this pull request Jul 11, 2023
Replace IndexedGzipFile.__reduce__ with IndexedGzipFile.__reduce_ex__
4a6b3a1 
Restores “pickle-ability” of IndexedGzipFile in Python 3.12, which was
broken due to python/cpython#101948.

Fixes pauldmccarthy#125.
serhiy-storchaka mentioned this pull request Aug 2, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

kumaraditya303 kumaraditya303 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL