fix: to_graphml crashes on dict/list-valued graph, node, and edge attributes#1830
fix: to_graphml crashes on dict/list-valued graph, node, and edge attributes#1830hofmockel wants to merge 1 commit into
Conversation
…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.
There was a problem hiding this comment.
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.pypasses on this branch: 45 tests passed. -
I reproduced a remaining failure using a
MultiDiGraphwith node metadata and a list-valued edge attribute. The export fails beforenx.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 forGraph/DiGraph, but a multigraph edge is identified by(u, v, key). -
This appears in scope because Graphify has explicit
MultiGraph/MultiDiGraphcompatibility 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
MultiDiGraphregression 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:
_scrubcould be renamed to_normalize_graphml_valueor_to_graphml_attribute_valuefor clarity.
I left the suggested implementation directly on the affected edge loop.
| if val is None: | ||
| H.nodes[node_id][key] = "" | ||
| H.nodes[node_id][key] = _scrub(val) | ||
| for u, v in H.edges(): |
There was a problem hiding this comment.
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:
| 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.
|
Suggested test cases for this change:
The minimum merge-blocking set is |
|
Thanks @hofmockel — landed in |
Summary
nx.write_graphmlonly accepts scalar attribute values.to_graphmlalready coercesNone -> ""(fixing a priorValueError, #1502), but adictorlistvalue still raises:This hits real graphs in practice, not just synthetic edge cases:
"metadata"dict attributeattach_hyperedges()'s graph-level"hyperedges"list (G.graph["hyperedges"])Both crash
graphify export graphmloutright 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 coercesNone -> "") to also JSON-serialize any dict/list-valued attribute — across graph-level, node, and edge scopes — before callingnx.write_graphml.Test plan
test_to_graphml_tolerates_dict_and_list_attribute_values(tests/test_export.py), mirroring the existingtest_to_graphml_tolerates_none_attribute_valuestest — injects a nodemetadatadict, an edgelistattribute, and a graph-levelhyperedgeslist, assertsto_graphmldoesn't raise.tests/test_export.py(all passing) and the full suite (57 failed, 2891 passed, 165 skipped— the 57 failures are pre-existing, all intest_read_hook.py/test_reflect.py/test_search_hook.py/test_terraform.py, unrelated toexport.py/GraphML and present before this change).networkx.read_graphml.