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.

Sign up for GitHub

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

correct labels for dicts #50

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

correct labels for dicts #50

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a931810
correct labels for dicts
graingert Aug 17, 2020
bcbc763
make _edge_labels a generator
graingert Aug 19, 2020
1d921d5
Apply suggestions from code review
graingert Mar 28, 2022
34b5072
Update objgraph.py
graingert Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 47 additions & 26 deletions objgraph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@
# Python 3.x compatibility
iteritems = dict.items

try:
zip_longest = itertools.izip_longest
except AttributeError: # pragma: PY3
# Python 3.x compatibility
zip_longest = itertools.zip_longest
Copy link
Author

Choose a reason for hiding this comment

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

can we use six.moves.zip_longest instead?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't want to introduce non-optional dependencies outside Python's stdlib yet.


graingert marked this conversation as resolved.
Show resolved Hide resolved
IS_INTERACTIVE = False
try: # pragma: nocover
import graphviz
Expand Down Expand Up @@ -1000,7 +1006,10 @@ def _show_graph(objs, edge_func, swap_source_target,
continue
if cull_func is not None and cull_func(target):
continue
neighbours = edge_func(target)
edges = edge_func(target)
graingert marked this conversation as resolved.
Show resolved Hide resolved
counts = collections.Counter(id(v) for v in edges)
neighbours = list({id(v): v for v in edges}.values())
del edges
Copy link
Owner

Choose a reason for hiding this comment

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

So AFAIU neighbours is edges without duplicates, and counts is the count of duplicates. This took me a while to understand, maybe a comment would be helpful?

Copy link
Author

Choose a reason for hiding this comment

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

no need for a comment if we can add more_itertools:

https://more-itertools.readthedocs.io/en/stable/api.html#more_itertools.unique_everseen

neighbours = more_itertools.unique_everseen(edges, key=id)

Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather not; I like that objgraph is a single file library with no external dependencies I can copy over into any random location on my python path and start using immediately.

ignore.add(id(neighbours))
n = 0
skipped = 0
Expand All @@ -1016,9 +1025,13 @@ def _show_graph(objs, edge_func, swap_source_target,
srcnode, tgtnode = target, source
else:
srcnode, tgtnode = source, target
elabel = _edge_label(srcnode, tgtnode, shortnames)
f.write(' %s -> %s%s;\n' % (_obj_node_id(srcnode),
_obj_node_id(tgtnode), elabel))
for elabel, _ in zip_longest(
graingert marked this conversation as resolved.
Show resolved Hide resolved
_edge_labels(srcnode, tgtnode, shortnames),
range(counts[id(source)]),
fillvalue='',
):
graingert marked this conversation as resolved.
Show resolved Hide resolved
f.write(' %s -> %s%s;\n' % (_obj_node_id(srcnode),
_obj_node_id(tgtnode), elabel))
if id(source) not in depth:
depth[id(source)] = tdepth + 1
queue.append(source)
Copy link
Owner

Choose a reason for hiding this comment

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

A couple of lines down from here there's a del neighbours, and I think you might want to add del counts in there.

It probably doesn't matter, counts holds only ints, and I doubt anyone expects specific refcounts for those, and also they won't be causing problems with memory usage.

Copy link
Author

Choose a reason for hiding this comment

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

yeah I figured it wasn't worth it, could wrap this whole thing in a def so everything is automatically deleted

Expand Down Expand Up @@ -1208,43 +1221,51 @@ def _gradient(start_color, end_color, depth, max_depth):
return h, s, v


def _edge_label(source, target, shortnames=True):
def _edge_labels(source, target, shortnames=True):
if (_isinstance(target, dict)
and target is getattr(source, '__dict__', None)):
return ' [label="__dict__",weight=10]'
return [' [label="__dict__",weight=10]']
if _isinstance(source, types.FrameType):
if target is source.f_locals:
return ' [label="f_locals",weight=10]'
return [' [label="f_locals",weight=10]']
if target is source.f_globals:
return ' [label="f_globals",weight=10]'
return [' [label="f_globals",weight=10]']
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if there are any frames where frame.f_locals is frame.f_globals and we'd have the same problem with two arrows having the same label?

I wonder if it wouldn't be a good idea to use yield here for easier handling of multiple return values.

Copy link
Author

Choose a reason for hiding this comment

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

I tried using yield but it breaks the control flow around return

Copy link
Author

Choose a reason for hiding this comment

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

oh I see yes that might be much better

Copy link
Owner

@mgedmin mgedmin Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It would require replacing all return statements with yield, and then adding return with no arguments for short-circuiting the control flow (or rewriting the series of top-level 'if/if/if' into 'if/elif/elif').

Not worth it. When I first suggested yield, I thought there would be more places with possible duplicate labels, but it looks like just this one place.

Copy link
Author

Choose a reason for hiding this comment

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

lol I already did it

if _isinstance(source, types.MethodType):
try:
if target is source.__self__:
return ' [label="__self__",weight=10]'
return [' [label="__self__",weight=10]']
if target is source.__func__:
return ' [label="__func__",weight=10]'
return [' [label="__func__",weight=10]']
except AttributeError: # pragma: nocover
# Python < 2.6 compatibility
if target is source.im_self:
return ' [label="im_self",weight=10]'
return [' [label="im_self",weight=10]']
if target is source.im_func:
return ' [label="im_func",weight=10]'
return [' [label="im_func",weight=10]']
if _isinstance(source, types.FunctionType):
for k in dir(source):
if target is getattr(source, k):
return ' [label="%s",weight=10]' % _quote(k)
return [
' [label="%s",weight=10]' % _quote(k)
for k in dir(source)
if target is getattr(source, k)
graingert marked this conversation as resolved.
Show resolved Hide resolved
]
Copy link
Owner

Choose a reason for hiding this comment

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

👍

if _isinstance(source, dict):
for k, v in iteritems(source):
if v is target:
if _isinstance(k, basestring) and _is_identifier(k):
return ' [label="%s",weight=2]' % _quote(k)
else:
if shortnames:
tn = _short_typename(k)
else:
tn = _long_typename(k)
return ' [label="%s"]' % _quote(tn + "\n" + _safe_repr(k))
return ''
tn = _short_typename if shortnames else _long_typename
return [
(
' [label="%s",weight=2]' % _quote(k)
if _isinstance(k, basestring) and _is_identifier(k)
else (
' [label="%s"]' % _quote(tn(k) + "\n" + _safe_repr(k))
)
)
for k, v in iteritems(source)
if v is target
]
return []
graingert marked this conversation as resolved.
Show resolved Hide resolved


def _edge_label(*args, **kwargs):
return next(iter(_edge_labels(*args, **kwargs)), '')
graingert marked this conversation as resolved.
Show resolved Hide resolved


_is_identifier = re.compile('[a-zA-Z_][a-zA-Z_0-9]*$').match
Expand Down
34 changes: 34 additions & 0 deletions tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,40 @@ def test_cull_func(self):
label_a=label_a,
label_b=label_b))

@skipIf(
sys.version_info < (3, 6),
"Python < 3.6 dicts have random iteration order",
)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm tempted to do a quick hack with

lines = output.readlines()
self.assertEqual(
    ''.join(lines[:5] + sorted(lines[5:-3]) + lines[-3:]),
    textwrap.dedent(...)
)

so this test can run on all Python versions.

Copy link
Author

Choose a reason for hiding this comment

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

but the test looks so beautiful without it

Copy link
Owner

Choose a reason for hiding this comment

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

The rest of the test will continue to look beautiful, and the sorted(lines[5:-3]) will inspire awe in the unwary reader. ;)

Copy link
Owner

Choose a reason for hiding this comment

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

Python 3.5 reached End Of Life recently, so the point is moot.

graingert marked this conversation as resolved.
Show resolved Hide resolved
def test_dict(self):
d = dict.fromkeys("abcdefg")
output = StringIO()
objgraph.show_refs(d, output=output)
self.assertEqual(
output.getvalue(),
textwrap.dedent(
"""\
digraph ObjectGraph {{
node[shape=box, style=filled, fillcolor=white];
{d_id}[fontcolor=red];
{d_id}[label="dict\\n7 items"];
{d_id}[fillcolor="0,0,1"];
{d_id} -> {none_id} [label="a",weight=2];
{d_id} -> {none_id} [label="b",weight=2];
{d_id} -> {none_id} [label="c",weight=2];
{d_id} -> {none_id} [label="d",weight=2];
{d_id} -> {none_id} [label="e",weight=2];
{d_id} -> {none_id} [label="f",weight=2];
{d_id} -> {none_id} [label="g",weight=2];
{none_id}[label="NoneType\\nNone"];
{none_id}[fillcolor="0,0,0.766667"];
}}
"""
).format(
d_id=objgraph._obj_node_id(d),
none_id=objgraph._obj_node_id(None),
),
)

@mock.patch('objgraph.IS_INTERACTIVE', True)
@mock.patch('objgraph.graphviz', create=True)
def test_ipython(self, mock_graphviz):
Expand Down