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 — gh-91052: Add C API for watching dictionaries by carljm · Pull Request #31787 · python/cpython

Conversation

@carljm

@carljm carljm commented Mar 9, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Notes on the PR for reviewers:

  • Since this API is intended for use by extensions, and _Py* APIs are internal-only, I added the API as public (Py*). But I think it should perhaps be CPython-specific, so I put it in Include/cpython/dictobject.h, not Include/dictobject.h. Not sure about any of this, though, so happy to change as requested.
  • Issue: add support for watching writes to selected dictionaries #91052

Sorry, something went wrong.

Uh oh!

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

All reactions
@carljm
carljm requested review from markshannon and methane as code owners March 9, 2022 14:31
@bedevere-bot bedevere-bot added the awaiting review label Mar 9, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 9, 2022
itamaro
itamaro reviewed Mar 9, 2022
View reviewed changes
Comment thread Objects/dictobject.c Resolved

Uh oh!

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

Comment thread Objects/dictobject.c Resolved

Uh oh!

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

@carljm
carljm force-pushed the dictwatch branch 3 times, most recently from eed27b4 to d22463b Compare March 14, 2022 23:14
@MaxwellDupre

MaxwellDupre commented May 2, 2022

Copy link
Copy Markdown
Contributor

Do you have a test harness/or something I can use (without too much effort :)? I could just run the Python test suite but that may be missing the point.

All reactions

Sorry, something went wrong.

Uh oh!

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

@carljm

carljm commented May 2, 2022

Copy link
Copy Markdown
Member Author

Do you have a test harness/or something I can use (without too much effort :)? I could just run the Python test suite but that may be missing the point.

Hi! I'm not sure what you are trying to achieve -- can you clarify your goals and how they relate to this PR?

All reactions

Sorry, something went wrong.

Uh oh!

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

@carljm carljm mentioned this pull request May 3, 2022
add support for watching writes to selected dictionaries #91052
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@ghost

ghost commented Oct 3, 2022 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

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

@carljm
carljm force-pushed the dictwatch branch from 4623234 to 11caa9e Compare October 3, 2022 23:06
@carljm
pythongh-91052: Add C API for watching dictionaries
0447232
@carljm
carljm force-pushed the dictwatch branch from 11caa9e to 0447232 Compare October 3, 2022 23:10
@carljm carljm changed the title bpo-46896: Add C API for watching dictionaries gh-91052: Add C API for watching dictionaries Oct 3, 2022
@carljm

carljm commented Oct 4, 2022

Copy link
Copy Markdown
Member Author

@markshannon I've updated this PR to the design we discussed, would be glad for your review. I'll try to get some updated pyperformance numbers tomorrow.

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm added 2 commits October 4, 2022 09:51
@carljm
Add second-watcher tests, fix MSVC warning
1c7f814
@carljm
Over-engineer the watcher bits check
6e5cee2
@carljm

carljm commented Oct 4, 2022

Copy link
Copy Markdown
Member Author

Shows neutral PyPerformance results: https://gist.github.com/carljm/fa125fe5147146cd62ae10485b3fc31b

All reactions

Sorry, something went wrong.

Uh oh!

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

Fidget-Spinner
Fidget-Spinner reviewed Oct 5, 2022
View reviewed changes
Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

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

markshannon
markshannon reviewed Oct 5, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Generally looks good. A few comments.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Include/internal/pycore_dict.h Resolved

Uh oh!

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

Comment thread Include/internal/pycore_interp.h Resolved

Uh oh!

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

Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

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

Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

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

Comment thread Modules/_testcapimodule.c Resolved

Uh oh!

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

carljm added 4 commits October 5, 2022 15:04
@carljm
Merge branch 'main' into dictwatch
4bb5d9d
* main: (66 commits)
  pythongh-65961: Raise `DeprecationWarning` when `__package__` differs from `__spec__.parent` (python#97879)
  docs(typing): add "see PEP 675" to LiteralString (python#97926)
  pythongh-97850: Remove all known instances of module_repr() (python#97876)
  I changed my surname early this year (python#96671)
  pythongh-93738: Documentation C syntax (:c:type:<C type> -> :c:expr:<C type>) (python#97768)
  pythongh-91539: improve performance of get_proxies_environment  (python#91566)
  build(deps): bump actions/stale from 5 to 6 (python#97701)
  pythonGH-95172 Make the same version `versionadded` oneline (python#95172)
  pythongh-88050: Fix asyncio subprocess to kill process cleanly when process is blocked (python#32073)
  pythongh-93738: Documentation C syntax (Function glob patterns -> literal markup) (python#97774)
  pythongh-93357: Port test cases to IsolatedAsyncioTestCase, part 2 (python#97896)
  pythongh-95196: Disable incorrect pickling of the C implemented classmethod descriptors (pythonGH-96383)
  pythongh-97758: Fix a crash in getpath_joinpath() called without arguments (pythonGH-97759)
  pythongh-74696: Pass root_dir to custom archivers which support it (pythonGH-94251)
  pythongh-97661: Improve accuracy of sqlite3.Cursor.fetchone docs (python#97662)
  pythongh-87092: bring compiler code closer to a preprocessing-opt-assembler organisation (pythonGH-97644)
  pythonGH-96704: Add {Task,Handle}.get_context(), use it in call_exception_handler() (python#96756)
  pythongh-93738: Documentation C syntax (:c:type:`PyTypeObject*` -> :c:expr:`PyTypeObject*`) (python#97778)
  pythongh-97825: fix AttributeError when calling subprocess.check_output(input=None) with encoding or errors args (python#97826)
  Add re.VERBOSE flag documentation example (python#97678)
  ...
@carljm
Address review comments
7dcf9b0
@carljm
Merge branch 'main' into dictwatch
7938d6f
* main:
  pythonGH-88050: fix race in closing subprocess pipe in asyncio  (python#97951)
  pythongh-93738: Disallow pre-v3 syntax in the C domain (python#97962)
  pythongh-95986: Fix the example using match keyword (python#95989)
  pythongh-97897: Prevent os.mkfifo and os.mknod segfaults with macOS 13 SDK (pythonGH-97944)
  pythongh-94808: Cover `PyUnicode_Count` in CAPI (python#96929)
  pythongh-94808: Cover `PyObject_PyBytes` case with custom `__bytes__` method (python#96610)
  pythongh-95691: Doc BufferedWriter and BufferedReader (python#95703)
  pythonGH-88968: Add notes about socket ownership transfers (python#97936)
  pythongh-96865: [Enum] fix Flag to use CONFORM boundary (pythonGH-97528)
@carljm
Minor cleanups
fb19617
@carljm carljm mentioned this pull request Oct 6, 2022
gh-91051: allow setting a callback hook on PyType_Modified #97875
Merged
markshannon
markshannon reviewed Oct 6, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A few more comments, but looking good.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/c-api/dict.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/dict.rst Resolved

Uh oh!

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

Comment thread Doc/c-api/dict.rst Resolved

Uh oh!

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

Comment thread Include/cpython/dictobject.h Resolved

Uh oh!

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

@carljm
Merge branch 'main' into dictwatch
e5d2169
* main:
  fixes pythongh-96078: os.sched_yield release the GIL while calling sched_yield(2). (pythongh-97965)
  pythongh-65961: Do not rely solely on `__cached__` (pythonGH-97990)
  pythongh-97850: Remove the open issues section from the import reference (python#97935)
  Docs: pin sphinx-lint (pythonGH-97992)
  pythongh-94590: add signatures to operator itemgetter, attrgetter, methodcaller (python#94591)
  Add Pynche's move to the What's new in 3.11 (python#97974)
  pythongh-97781: Apply changes from importlib_metadata 5. (pythonGH-97785)
  pythongh-86482: Document assignment expression need for ()s (python#23291)
  pythongh-97943: PyFunction_GetAnnotations should return a borrowed reference. (python#97949)
  pythongh-94808: Coverage: Test that maximum indentation level is handled (python#95926)
@carljm
Address further review comments
cc1d0b7
@markshannon

markshannon commented Oct 7, 2022

Copy link
Copy Markdown
Member

LGTM, thanks.

tekknolagi reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

@markshannon
markshannon merged commit a4b7794 into python:main Oct 7, 2022
@bedevere-bot bedevere-bot removed the awaiting review label Oct 7, 2022
@carljm
carljm deleted the dictwatch branch October 7, 2022 00:14
@sweeneyde

sweeneyde commented Oct 7, 2022

Copy link
Copy Markdown
Member

I think this may have introduced some refleaks into test_capi.

All reactions

Sorry, something went wrong.

Uh oh!

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

@carljm

carljm commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

Thanks for the heads up, I'll take a look.

All reactions

Sorry, something went wrong.

Uh oh!

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

@carljm carljm mentioned this pull request Oct 7, 2022
Fix memory leaks in test_capi #98017
Merged
iritkatriel
iritkatriel reviewed Oct 7, 2022
View reviewed changes
Comment thread Lib/test/test_capi.py
self.assertIs(unraisable.object, d)
self.assertEqual(str(unraisable.exc_value), "boom!")

def test_two_watchers(self):

@iritkatriel iritkatriel Oct 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This test is ref leaking on my PR: #98001

Sorry, something went wrong.

Uh oh!

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

All reactions

@iritkatriel iritkatriel Oct 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah probably fixed in #98017 ?

Sorry, something went wrong.

Uh oh!

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

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@vstinner

vstinner commented Oct 7, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

For the C API documentation changes, you should add .. versionadded:: 3.12 to new functions, and document them in Doc/whatsnew/3.12.rst:
https://docs.python.org/dev/whatsnew/3.12.html#id2

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@carljm carljm mentioned this pull request Oct 7, 2022
gh-91052: Add PyDict_Unwatch for unwatching a dictionary #98055
Merged
@carljm

carljm commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

Thanks @vstinner , I included these changes in #98055.

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
29e38df
* main:
  bpo-35540 dataclasses.asdict now supports defaultdict fields (pythongh-32056)
  pythonGH-91052: Add C API for watching dictionaries (pythonGH-31787)
  bpo-38693: Use f-strings instead of str.format() within importlib (python#17058)
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@carljm @mpage
pythonGH-91052: Add C API for watching dictionaries (pythonGH-31787)
4f9e610
@erlend-aasland erlend-aasland mentioned this pull request Nov 10, 2022
Split up _testcapimodule.c #93649
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@itamaro itamaro itamaro left review comments
@iritkatriel iritkatriel iritkatriel left review comments
@markshannon markshannon markshannon left review comments
@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments
@methane methane Awaiting requested review from methane methane is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

11 participants

@carljm @MaxwellDupre @markshannon @sweeneyde @vstinner @itamaro @iritkatriel @Fidget-Spinner @the-knights-who-say-ni @ezio-melotti @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.