New integrations with AHORN and Hypergraphx-Data - #759
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #759 +/- ##
==========================================
+ Coverage 94.43% 94.69% +0.25%
==========================================
Files 65 65
Lines 5213 5336 +123
==========================================
+ Hits 4923 5053 +130
+ Misses 290 283 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hey @leotrs --- (Sorry for the re-ping; I mistakenly deleted my previous comment) if you would be willing to help with unit tests and reviewing, I would really appreciate it! This seems like an ideal place for automated code generation. |
leotrs
left a comment
There was a problem hiding this comment.
Direction is right (this is the second half of the #756 split), and generalizing request_json_from_url into request_from_url(mode=...) is a nice cleanup. A few things I'd like to see addressed before merge.
Blockers
-
hypergraphx_data.py::_downloadreintroduces its own HTTP client withverify_ssl=Falseby default. This bypasses TLS certificate verification (ssl._create_unverified_context()), which opens the loader to man-in-the-middle attacks — every HGX download fromcricca.disi.unitn.itis served over an unverified TLS channel. On top of that, the function shadows the perfectly goodrequest_from_url(url, mode="raw")this same PR generalizes, and theverify_ssl=Truebranch is dead code (verify_ssldefaults toFalseand there's no public way to override it). Options: (a) fix the cert issue upstream or installcertifiand require it; (b) if there's a specific cert-chain problem with cricca.disi that can't be solved, handle it once insiderequest_from_urlwith a clear comment naming the host. Either way, don't ship a silent MITM path. If we merge as-is, users pulling HGX datasets are trusting whatever intermediary is on their network. -
load_ahorn_datahits the catalog twice per call. Once at the top of the function to buildindex_data, and again inside_get_dataset_data. That's an extra round trip toahorn.rwth-aachen.defor every non-listing invocation. Either fetch once and thread the catalog through_get_dataset_data, or route both throughrequest_from_url_cached. -
load_hypergraphx_datahas nocacheparameter.load_xgi_data,load_bigg_data, andload_ahorn_dataall do. Users shouldn't have to guess which loaders cache. Add it. -
_from_ahorn_textignoresnodetypeandedgetype. It always casts node IDs toint(node = int(ids)) and never appliesedgetypeto the edge ornodetypeto nodes/edge members. The HIF path does the right thing; the ahorn-text path silently drops the caller's contract. If AHORN datasets can come back in either format (they can, per_get_dataset_url), the two paths need to be consistent.
Should fix (not blocking but worth doing)
-
Exception types don't match docstrings. Both
load_ahorn_dataandload_hypergraphx_datadocstrings say "Raises XGIError" for invalid dataset names, but the code raisesKeyError. Either useXGIError(matches the rest of the loaders) or fix the docstring. Same file:_parse_remote_dataset_catalograisesTypeErrorfor parsing/validation failures, whereXGIErrororValueErroris more appropriate —TypeErrorshould be reserved for actual type mismatches. -
The HGX catalog parser is a fragility waiting to happen.
_parse_remote_dataset_catalogregex-matcheswindow.RELATED_DATASETS = ...out of a JavaScript file. If the hypergraphx-data team refactors their static JS (renames the global, minifies it, splits it, wraps it in an IIFE, anything), we silently break. This is exactly the "maintenance contract" concern I raised on #756 — worth pinging the HGX maintainers to see if they can expose a stable JSON endpoint instead. Not blocking this PR, but file an issue so it's tracked.
Nits
-
_get_dataset_urlaccepts arevisionargument but there's no way to pass it fromload_ahorn_data— always defaults to max. Either wire it through or drop the parameter until it has a caller. -
load_ahorn_datamixesprintandreturnon the listing path —print(*index_data, sep="\n")and thenreturn index_data. Matchesload_xgi_data's pattern, so consistent, but worth flagging as a broader-API cleanup later. -
Docstring for the new
modeparameter inrequest_from_urlandrequest_from_url_cachedis duplicated verbatim. Fine, but a shared constant or aSee Alsoon one would DRY it. -
nodetypein_load_hypergraph(HGX) — applied tonode_idand to each element ofinteraction, but the edgemetadata.pop("id")path usesedgetypecorrectly. That part looks fine, but skim it in case any metadata field carries node references. -
Coverage check is failing (
codecov/project:FAILURE). Not blocking, but a quick look at what's uncovered would probably surface some of the above (the_downloaderror paths aren't tested, neither is theformat == "ahorn"branch in_request_from_ahorn_data). -
Benchmark workflow bumped to Python 3.14 without confirming the benchmark deps install cleanly on 3.14 (asv, matplotlib, scipy…). If any of them lag, benchmarks silently break.
Overall the loaders are the right shape and the utility rename is clean. Please address 1-4 and I'll re-review; 5-6 would be nice too.
The `except HTTPError` branch in `_download` re-raised a new HTTPError with only the message argument, but HTTPError's __init__ signature is (url, code, msg, hdrs, fp). Any real HTTP failure would therefore fall into a TypeError inside the exception handler rather than surface the intended message. Pass the original exception's url/code/headers through and use the wrapped-message string as msg. Surfaced by the new unit test for this branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyLLuAmW2dQ7KCewxArJHd
Adds targeted mocked tests to cover the paths that the existing webtest markers skip locally: AHORN (test_ahorn_data.py): - listing branch of load_ahorn_data with prints - KeyError on invalid dataset name - happy path through the HIF format - cache=False routes through the uncached utility - ahorn-text format branch of _request_from_ahorn_data - max_order truncation - _from_ahorn_text: metadata + nodes + edges, blank-line skipping hypergraphx-data (test_hypergraphx_data.py): - edge record without an explicit id - unknown record-type is silently ignored - _parse_remote_dataset_catalog error branches: invalid JSON, invalid window.RELATED_DATASETS body, non-list shape, name-less items skipped - load_hypergraphx_data listing, invalid dataset, full dispatch, max_order truncation - _download happy path, HTTPError wrapping, URLError wrapping Uses mocked responses and gzipped fixtures throughout, so nothing hits the network. All 35 tests in the two files pass under the current matrix Python versions (previously most of them were webtest-skipped). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyLLuAmW2dQ7KCewxArJHd
|
Pushed two commits to your branch: 01a5b15 fix: construct HTTPError with the required positional args in _download 4440a86 test: add unit tests for AHORN and hypergraphx-data loaders AHORN:
hypergraphx-data:
Full local suite: 480 passed / 7 skipped. Codecov should be a lot happier — most of the previously-untested lines are now covered without needing the network. The review points on |
Applies four review comments from #759: 1. `load_ahorn_data` now fetches the AHORN catalog exactly once per call and threads it into `_request_from_ahorn_data` / `_get_dataset_data`. Previously the catalog was fetched twice (once at the top of `load_ahorn_data` and again inside `_get_dataset_data`). Kept the `catalog=None` path in `_get_dataset_data` so direct callers still work. 2. `load_hypergraphx_data` gains a `cache=True` parameter, matching `load_xgi_data`, `load_ahorn_data`, and `load_bigg_data`. Caching is implemented as `@cache _download_cached(url)` wrapping the existing `_download`, so the underlying network path is unchanged. 3. `_from_ahorn_text` now honors `nodetype` for node IDs (both single nodes and edge members). Defaults to `int`, preserving the existing behavior when `nodetype` is not passed. `edgetype` remains unused because edges get auto-assigned uids from `Hypergraph`; documented this in the parameter docstring. 4. `load_ahorn_data` and `load_hypergraphx_data` now raise `XGIError` for invalid dataset names, matching their docstrings (previously they raised `KeyError`). Error messages name the specific repository. Tests updated accordingly and new coverage added: - `test_load_ahorn_data_fetches_catalog_once` — asserts single catalog hit. - `test_load_ahorn_data_invalid_dataset` — asserts XGIError. - `test_from_ahorn_text_respects_nodetype` — asserts nodetype casting. - `test_load_hypergraphx_data_invalid_dataset` — asserts XGIError. - `test_load_hypergraphx_data_cache_false_bypasses_cache` — asserts the cache=False path routes around the memoized wrapper. - `test_load_hypergraphx_data_cache_true_reuses_result` — asserts two calls with cache=True hit `_download` only once. Full local suite: 484 passed / 7 skipped. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyLLuAmW2dQ7KCewxArJHd
|
One more commit addressing the remaining low-risk review items: 933805d refactor: address review items on AHORN and hypergraphx-data loaders
Left explicitly for you:
Full local suite: 484 passed / 7 skipped. Adding this brings us to a total of +39 mocked tests on top of the two you had. |
The pre-existing `test_load_ahorn_data` webtest asserted `KeyError` for the invalid-dataset path. The previous commit changed `load_ahorn_data` to raise `XGIError` to match its docstring. Local tests were updated in that commit but the webtest was skipped locally (network-required), so the mismatch was only caught by Ubuntu CI. Update it too. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LyLLuAmW2dQ7KCewxArJHd
|
CI is green (20/20). Here's the final state of the review items: Addressed in-branch:
Left for you:
Please give the last three commits (01a5b15, 4440a86, 933805d, ac9b395) a look and merge when you're happy. |
This PR does the following:
request_json_from_urlandrequest_json_from_url_cachedtorequest_from_urlandrequest_from_url_cachedload_ahorn_dataandload_hypergraphx_datato the XGI API.