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-98169 dataclasses.astuple support DefaultDict by kwsp · Pull Request #98170 · python/cpython

Conversation

@kwsp

@kwsp kwsp commented Oct 11, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Basically applying the same fix for asdict from #32056 to astuple. These two should've been the same PR, but I didn't know about astuple previously.

Also modified the handling of DefaultDict in asdict a little so that we only call isinstance(obj, dict) once. This results in a ~3% speed up on my machine on this example:

# benchmark
from dataclasses import dataclass, asdict
from collections import defaultdict
from typing import DefaultDict, Dict, List, NamedTuple
import time


class NT(NamedTuple):
    x: int
    y: float

@dataclass
class C:
    dd: DefaultDict[str, List]
    nt: NT
    d1: Dict[str, List]
    d2: Dict[str, int]

def get_instance():
    instance = C(dd=defaultdict(list), nt=NT(1, 1.), d1={}, d2={})
    instance.dd["x"].append(12)
    instance.dd["x"].append(13)
    instance.d1["y"] = [1, 2, 3]
    instance.d2["z"] = 1
    return instance

instance = get_instance()

def timeit(callable, n_runs = 100_000, name=""):
    t1 = time.perf_counter_ns()
    for _ in range(n_runs):
        callable()

    elapsed_ms = round(1e-6 * (time.perf_counter_ns() - t1), 2)
    print(f"{name}\t{n_runs} runs: {elapsed_ms} ms")

timeit(lambda: asdict(instance), name="asdict")
timeit(lambda: astuple(instance), name="astuple")
  • Issue: dataclasses.astuple breaks on DefaultDict #98169

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kwsp
pythongh-98169 dataclasses.astuple support DefaultDict
81d9170
@kwsp
kwsp requested a review from ericvsmith as a code owner October 11, 2022 00:06
@bedevere-bot bedevere-bot added the awaiting review label Oct 11, 2022
kwsp added 2 commits October 10, 2022 19:12
@kwsp
removed links to the bugs
8aa1b3d
@kwsp
news.d
f39e3d8
eendebakpt
eendebakpt reviewed Oct 22, 2022
View reviewed changes
Comment thread Lib/dataclasses.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/dataclasses.py Resolved

Uh oh!

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

eendebakpt commented Mar 13, 2023

Copy link
Copy Markdown
Contributor

@kwsp Can you address the review comments?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kwsp @eendebakpt
Update Lib/dataclasses.py
fc6a5a9 
cache the output of `type(obj)` to avoid computing it 3 times.

Co-authored-by: Pieter Eendebak <[email protected]>
@bedevere-bot bedevere-bot mentioned this pull request Mar 13, 2023
dataclasses.astuple breaks on DefaultDict #98169
Closed
@kwsp

kwsp commented Mar 13, 2023

Copy link
Copy Markdown
Contributor Author

@eendebakpt sorry I missed the notification. Addressed your change.

Could you please comment on my response to if instanceof(obj, defaultdict)?
All reactions

Sorry, something went wrong.

Uh oh!

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

eendebakpt commented Mar 13, 2023

Copy link
Copy Markdown
Contributor

@eendebakpt sorry I missed the notification. Addressed your change.

Could you please comment on my response to if instanceof(obj, defaultdict)?

@kwsp I cannot see the comment
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
eendebakpt
eendebakpt reviewed Mar 13, 2023
View reviewed changes
Comment thread Lib/dataclasses.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@kwsp @eendebakpt
Update Lib/dataclasses.py
44aaee9 
Co-authored-by: Pieter Eendebak <[email protected]>
@kwsp

kwsp commented Mar 13, 2023

Copy link
Copy Markdown
Contributor Author

#98170 (comment)

I responded:

Good suggestion. I did this originally to be consistent with the existing code that checks for named tuples, and to avoid importing the colelctions module.

cpython/Lib/dataclasses.py

Line 1302 in f39e3d8

elif isinstance(obj, tuple) and hasattr(obj, '_fields'):

I originally assumed that for the namedtuple case, they could've done a is instance(obj, namedtuple) but didn't to avoid an import from from collections import namedtuple, but I just realised is instance(obj, namedtuple) wouldn't work because namedtuple is a factory function...

So now the question is should we import collections in this module? I assume it's imported elsewhere during Python startup so adding the import here should be free right?
All reactions

Sorry, something went wrong.

Uh oh!

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

eendebakpt commented Mar 13, 2023

Copy link
Copy Markdown
Contributor

I do not now the origins of the hasattr check, but I agree the import time of collections should not be an issue. But let's keep the code consistent for now and keep the hasattr just as in the asdict code.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
eendebakpt
eendebakpt approved these changes Mar 13, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Mar 13, 2023
carljm
carljm approved these changes Mar 13, 2023
View reviewed changes

@carljm carljm 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.

IMO it would probably be better to just do isinstance(obj, defaultdict) in both of these cases, but I also think that's a bit out of scope for this PR. The code here LGTM, seems like a straightforward extension of the feature that was already merged for asdict.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kwsp reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Mar 13, 2023
@carljm
carljm merged commit 71e37d9 into python:main Mar 13, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 13, 2023
carljm added a commit to carljm/cpython that referenced this pull request Mar 14, 2023
@carljm
Merge branch 'main' into inlinecomp2
6c5f269 
* main: (50 commits)
  pythongh-102674: Remove _specialization_stats from Lib/opcode.py (python#102685)
  pythongh-102660: Handle m_copy Specially for the sys and builtins Modules (pythongh-102661)
  pythongh-102354: change python3 to python in docs examples (python#102696)
  pythongh-81057: Add a CI Check for New Unsupported C Global Variables (pythongh-102506)
  pythonGH-94851: check unicode consistency of static strings in debug mode (python#102684)
  pythongh-100315: clarification to `__slots__` docs. (python#102621)
  pythonGH-100227: cleanup initialization of global interned dict (python#102682)
  doc: Remove a duplicate 'versionchanged' in library/asyncio-task (pythongh-102677)
  pythongh-102013: Add PyUnstable_GC_VisitObjects (python#102014)
  pythonGH-102670: Use sumprod() to simplify, speed up, and improve accuracy of statistics functions (pythonGH-102649)
  pythongh-102627: Replace address pointing toward malicious web page (python#102630)
  pythongh-98831: Use DECREF_INPUTS() more (python#102409)
  pythongh-101659: Avoid Allocation for Shared Exceptions in the _xxsubinterpreters Module (pythongh-102659)
  pythongh-101524: Fix the ChannelID tp_name (pythongh-102655)
  pythongh-102069: Fix `__weakref__` descriptor generation for custom dataclasses (python#102075)
  pythongh-98169 dataclasses.astuple support DefaultDict (python#98170)
  pythongh-102650: Remove duplicate include directives from multiple source files (python#102651)
  pythonGH-100987: Don't cache references to the names and consts array in `_PyEval_EvalFrameDefault`. (python#102640)
  pythongh-87092: refactor assemble() to a number of separate functions, which do not need the compiler struct (python#102562)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (python#102631)
  ...
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@kwsp @eendebakpt @Fidget-Spinner
pythongh-98169 dataclasses.astuple support DefaultDict (python#98170)
c54eeaf 
Co-authored-by: Pieter Eendebak <[email protected]>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@kwsp @eendebakpt @warsaw
pythongh-98169 dataclasses.astuple support DefaultDict (python#98170)
3f86f25 
Co-authored-by: Pieter Eendebak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes
@eendebakpt eendebakpt eendebakpt approved these changes
@ericvsmith ericvsmith Awaiting requested review from ericvsmith ericvsmith 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.

4 participants

@kwsp @eendebakpt @carljm @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.