Skip to content

fix: to_graphml crashes on dict/list-valued graph, node, and edge attributes#1830

Closed
hofmockel wants to merge 1 commit into
Graphify-Labs:v8from
hofmockel:fix/graphml-scalar-attribute-values
Closed

fix: to_graphml crashes on dict/list-valued graph, node, and edge attributes#1830
hofmockel wants to merge 1 commit into
Graphify-Labs:v8from
hofmockel:fix/graphml-scalar-attribute-values

Conversation

@hofmockel

Copy link
Copy Markdown

Summary

nx.write_graphml only accepts scalar attribute values. to_graphml already coerces None -> "" (fixing a prior ValueError, #1502), but a dict or list value still raises:

TypeError: GraphML does not support type <class 'dict'> as data values.
TypeError: GraphML does not support type <class 'list'> as data values.

This hits real graphs in practice, not just synthetic edge cases:

  • a per-node "metadata" dict attribute
  • attach_hyperedges()'s graph-level "hyperedges" list (G.graph["hyperedges"])

Both crash graphify export graphml outright on any graph that uses them — confirmed against a real ~2,300-node/4,300-edge project graph.

Fix

Extends the existing scrub step in to_graphml (the one that already coerces None -> "") to also JSON-serialize any dict/list-valued attribute — across graph-level, node, and edge scopes — before calling nx.write_graphml.

Test plan

  • Added test_to_graphml_tolerates_dict_and_list_attribute_values (tests/test_export.py), mirroring the existing test_to_graphml_tolerates_none_attribute_values test — injects a node metadata dict, an edge list attribute, and a graph-level hyperedges list, asserts to_graphml doesn't raise.
  • Ran full tests/test_export.py (all passing) and the full suite (57 failed, 2891 passed, 165 skipped — the 57 failures are pre-existing, all in test_read_hook.py/test_reflect.py/test_search_hook.py/test_terraform.py, unrelated to export.py/GraphML and present before this change).
  • Verified against a real project graph that previously crashed on export — now produces a valid file that round-trips through networkx.read_graphml.

…ributes

nx.write_graphml only accepts scalar attribute values. to_graphml already
coerces None -> "" (fixing a prior ValueError, Graphify-Labs#1502), but a dict or list
value still raises TypeError: GraphML does not support type <class 'dict'>
(or 'list') as data values. This hits real graphs in practice: a per-node
"metadata" dict, or attach_hyperedges()'s graph-level "hyperedges" list,
both crash `graphify export graphml` outright.

Fixed by extending the existing scrub step to also JSON-serialize any
dict/list-valued attribute (graph-level, node, and edge) before calling
nx.write_graphml, alongside the existing None handling.

Regression test: test_to_graphml_tolerates_dict_and_list_attribute_values
(tests/test_export.py), mirroring the existing None-value test. Verified
against a synthetic graph with a node metadata dict, an edge list attribute,
and a graph-level hyperedges list, and against a real 2,300+ node project
graph that previously crashed on all three.

@fyzanshaik fyzanshaik left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reproduced issue #1831 against this PR and came to this conclusion: the patch fixes GraphML serialization for regular Graph/DiGraph inputs, but to_graphml() still crashes when the input is a MultiGraph or MultiDiGraph. I believe the edge-scrubbing loop needs to handle NetworkX keyed edges before this is safe to merge.

  • The original issue is fixed for regular graphs: dict/list values at graph, node, and edge scope are converted to JSON strings.

  • tests/test_export.py passes on this branch: 45 tests passed.

  • I reproduced a remaining failure using a MultiDiGraph with node metadata and a list-valued edge attribute. The export fails before nx.write_graphml() runs:

ValueError: not enough values to unpack (expected 3, got 2)
  • The failure comes from accessing H.edges[u, v]. That lookup is valid for Graph/DiGraph, but a multigraph edge is identified by (u, v, key).

  • This appears in scope because Graphify has explicit MultiGraph/MultiDiGraph compatibility and node-link loading support. Issue #1831 asks for dict/list serialization across edge scope, which currently remains incomplete for this supported graph shape.

  • Please add a MultiDiGraph regression containing a dict/list-valued edge attribute. The test should read the generated GraphML back and assert that the edge attribute contains the expected JSON string, so it verifies preservation rather than only file creation.

  • Non-blocking: _scrub could be renamed to _normalize_graphml_value or _to_graphml_attribute_value for clarity.

I left the suggested implementation directly on the affected edge loop.

Comment thread graphify/export.py
if val is None:
H.nodes[node_id][key] = ""
H.nodes[node_id][key] = _scrub(val)
for u, v in H.edges():

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This lookup crashes for MultiGraph/MultiDiGraph because H.edges[u, v] requires an edge key. Iterating the mutable attribute dictionaries directly works for both regular and keyed graphs:

Suggested change
for u, v in H.edges():
for _, _, attrs in H.edges(data=True):
for key, value in list(attrs.items()):
attrs[key] = _scrub(value)

Please cover this with a MultiDiGraph edge containing a dict/list attribute and verify the value after a GraphML round trip.

@fyzanshaik

fyzanshaik commented Jul 12, 2026

Copy link
Copy Markdown

Suggested test cases for this change:

  • Graph: graph-level hyperedges list, node-level metadata dict, and edge-level list attribute export without raising.

  • DiGraph: the same values export successfully and preserve edge direction after nx.read_graphml().

  • MultiGraph: two parallel edges with different dict/list attributes both export; neither edge is skipped or overwritten.

  • MultiDiGraph: two keyed, directed parallel edges export successfully and remain distinct after reading the GraphML. This is the regression for the current failure.

  • Round trip: read the output with nx.read_graphml(), run json.loads() on each serialized dict/list value, and assert it equals the original value. Checking only that <graphml exists would not catch dropped or overwritten attributes.

  • Empty containers: {} and [] serialize to valid JSON strings and round-trip correctly.

  • Nested JSON values: a value such as {"items": [1, {"enabled": true}]} serializes and round-trips without structure loss.

  • Existing null handling: graph, node, and edge None attributes should still become ""; this prevents the current null-attribute behavior from regressing.

  • Input immutability: after to_graphml(), the original graph still contains dict/list objects rather than the JSON strings written through the copied graph.

  • Output validity: a successful export produces a non-empty file that nx.read_graphml() can parse.

The minimum merge-blocking set is Graph, MultiDiGraph with parallel edges, round-trip value assertions, and the existing None regression.

@safishamsi

Copy link
Copy Markdown
Collaborator

Thanks @hofmockel — landed in a46eee4 (0.9.14). I extended your approach slightly: coercion also covers the graph-level scope (where the hyperedges list lives) and preserves native int/float/bool rather than stringifying everything, plus an atomic temp-file write for the 0-byte-file case you called out in #1831. Credited you in the commit. Appreciate the PR and the mirrored test.

@safishamsi safishamsi closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants