Repository navigation
Conversation
Add one CSV partition per Node of a synthetic data set, a manifest with URLs, sizes and checksums, and the seeded generator that writes them. Tests check that the committed files match the generator. Add CI (lint, test, gate) for the Python code of the repository. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep LF endings of the test data, write the manifest with LF, say that the URLs follow main, and test the documented difference between the partitions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Run local summary statistics as one TES task per partition, directly or through a proTES gateway, read the aggregates from the task logs and combine them into the statistics of the whole data set. --local runs without services; tests use a fake TES service. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Accept --tes once or once per partition, add --poll and --timeout, count request time against the timeout, turn HTTP and network errors into a message and exit code 1, and limit the ANN401 exception to the demo and its tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Turn timeouts, connection errors and non-JSON bodies into task errors, and accept only positive, finite --poll and --timeout values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideData structuresAdd reproducible synthetic Node partitions and a manifest containing their locations, sizes, and checksums for the federated demo and its tests.
APIDefine the local-statistics and federated-analytics entry points, TES task and transport contract, CLI options, and aggregate-result formats.
ConfigurationSet up the Python project and locked development dependencies, CI lint/test/gate workflow, and repository file-handling rules.
DocumentationExplain the synthetic data and federated demo, their service requirements and quick start, and link the demo and CI instructions from the root README.
TestsCheck generated-data reproducibility, pooled-versus-partitioned analytics, TES client behavior and failures, and the documented local quick start.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
uniqueg
marked this pull request as ready for review
November 13, 2026 20:50
Linked issuesChecked at commit 055983e #5 demo: federated analytics · 1 of 3 met · 1 to check by hand
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 12 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="demos/federated-analytics/federated_analytics.py" line_range="221" />
<code_context>
+ if body is not None:
+ headers["Content-Type"] = "application/json"
+ data = json.dumps(body).encode()
+ if self.token:
+ headers["Authorization"] = f"Bearer {self.token}"
+ request = urllib.request.Request(url, data=data, headers=headers)
</code_context>
<issue_to_address>
**Bearer token exposed over HTTP**
When `TES_TOKEN` is set and the TES endpoint uses `http://`, `TesClient._request` adds the bearer token to the `Authorization` header and sends it unencrypted, so network observers can capture and use the token.
Reject plain-HTTP TES endpoints when a token is configured, or otherwise ensure authenticated requests use HTTPS.
Also at `demos/federated-analytics/federated_analytics.py:199-200`.
</issue_to_address>
### Comment 2
<location path="demos/federated-analytics/federated_analytics.py" line_range="182" />
<code_context>
+ Returns:
+ The decoded response body.
+ """
+ with urllib.request.urlopen(request, timeout=60) as response: # noqa: S310
+ return json.load(response)
+
</code_context>
<issue_to_address>
**Redirect leaks bearer token**
When an authenticated TES endpoint redirects a request to a different origin, `urlopen` follows redirects with the request headers, including the `Authorization` header added by `TesClient._request`; a redirect to another origin therefore sends the bearer token to that origin.
Reject cross-origin redirects or strip the authorization header before following them.
</issue_to_address>
### Comment 3
<location path="demos/federated-analytics/federated_analytics.py" line_range="273" />
<code_context>
+ while True:
+ task = self._request(f"{self.url}/{task_id}?view=FULL")
+ state = task.get("state", "UNKNOWN")
+ if state in TERMINAL_STATES:
+ break
+ if clock() >= deadline:
</code_context>
<issue_to_address>
**Task wait exceeds configured timeout**
When A status request or poll interval takes longer than the remaining timeout, `TesClient.wait` accepts terminal states before checking the deadline, sleeps for the full poll interval, and `_http` can block for its fixed 60-second timeout; the caller therefore waits past the configured timeout or sees success after it expires.
Check the deadline before accepting terminal states, cap each sleep and request timeout by the remaining budget, and fail when the deadline expires.
Also at `demos/federated-analytics/federated_analytics.py:182`, `demos/federated-analytics/federated_analytics.py:275-278`.
</issue_to_address>
### Comment 4
<location path="demos/federated-analytics/federated_analytics.py" line_range="243" />
<code_context>
+ Returns:
+ The ID of the task.
+ """
+ return str(self._request(self.url, task)["id"])
+
+ def wait(
</code_context>
<issue_to_address>
**Malformed TES response causes traceback**
When A TES endpoint returns valid JSON with a missing task ID or a non-object response, `TesClient.submit` indexes the response for `id`, and `TesClient.wait` calls `.get()` without validating the response shape; `KeyError` or `AttributeError` escapes `main`'s `TaskError` handling, so the caller sees a traceback.
Validate TES response shapes and required fields, and raise `TaskError` for invalid responses.
Also at `demos/federated-analytics/federated_analytics.py:272`, `demos/federated-analytics/federated_analytics.py:318`.
</issue_to_address>
### Comment 5
<location path="demos/federated-analytics/local_stats.py" line_range="45" />
<code_context>
+ group = row[GROUP_COLUMN]
+ values = {col: float(row[col]) for col in VALUE_COLUMNS}
+ except (KeyError, TypeError, ValueError) as exc:
+ raise ValueError(f"invalid row {row!r}: {exc}") from exc
+ per_group = stats.setdefault(group, {})
+ for col, value in values.items():
</code_context>
<issue_to_address>
**Malformed rows escape in task logs**
When a partition contains a row with a missing required field or a non-numeric value, `summarise` embeds the full row in its `ValueError`, and `main` leaves that exception uncaught. A malformed input therefore puts raw row contents in the executor traceback and TES stderr logs, sending data off the Node despite the claim that only aggregates leave it.
Report only a row index and field-level error, without including row values or chained conversion errors in task logs.
</issue_to_address>
### Comment 6
<location path="data/manifest.json" line_range="17" />
<code_context>
+ {
+ "node": "node-a",
+ "path": "synthetic-growth/node-a.csv",
+ "url": "https://raw.githubusercontent.com/elixir-cloud-aai/elixir-cloud-demos/main/data/synthetic-growth/node-a.csv",
+ "sha256": "425161d3670348f82b5fa1144dc9528b236c5a02fab4e609d5bbcd02fcff0f0f",
+ "size": 1626
</code_context>
<issue_to_address>
**Tasks can mix dataset revisions**
When the partition files on `main` change after the manifest is loaded or between task input downloads, `make_task` passes mutable `main` URLs to TES without verifying the manifest checksum, so tasks can summarize different dataset revisions and `combine` reports a hybrid result as successful.
Use immutable, revision-pinned input URLs for every task and verify the downloaded inputs against the manifest checksums.
Also at `demos/federated-analytics/federated_analytics.py:161`.
</issue_to_address>
### Comment 7
<location path="scripts/generate_test_data.py" line_range="112-114" />
<code_context>
+ for node in NODES:
+ rel_path = f"{DATASET}/{node}.csv"
+ content = _to_csv(make_rows(node=node))
+ target = out_dir / rel_path
+ target.parent.mkdir(parents=True, exist_ok=True)
+ target.write_bytes(content)
+ files.append(
+ {
</code_context>
<issue_to_address>
**Readers see partial dataset updates**
When a demo reads the data while generation is in progress, or generation stops after overwriting only some files, `write_data_sets` overwrites partition files in place and writes the manifest only after both files; a concurrent demo can read a truncated or mixed set of partitions and return incorrect statistics, or a failed generation can leave files inconsistent with the old manifest.
Write the complete dataset and manifest to a staging location, then publish them atomically as a unit.
</issue_to_address>
### Comment 8
<location path="demos/federated-analytics/local_stats.py" line_range="41-52" />
<code_context>
+ if self.token:
+ headers["Authorization"] = f"Bearer {self.token}"
+ request = urllib.request.Request(url, data=data, headers=headers)
+ try:
+ return self.transport(request)
+ except urllib.error.HTTPError as exc:
</code_context>
<issue_to_address>
**Empty partitions produce successful incomplete results**
When A partition contains only a header or no data rows, `local_stats.summarise` returns an empty mapping, which `combine` preserves or skips; the CLI returns success with an empty result or one that omits a Node's statistics.
Reject empty partitions or report them as an error instead of returning successful incomplete results.
Also at `demos/federated-analytics/federated_analytics.py:81-90`, `demos/federated-analytics/federated_analytics.py:105`, `demos/federated-analytics/federated_analytics.py:423-424`.
</issue_to_address>
### Comment 9
<location path="demos/federated-analytics/federated_analytics.py" line_range="423" />
<code_context>
+ print(f"Error: {exc}", file=sys.stderr)
+ return 1
+ result = combine(partials)
+ print(json.dumps(result, indent=2, sort_keys=True) if args.json else report(result))
+ return 0
+
</code_context>
<issue_to_address>
**JSON output contains invalid NaN**
When A group and column have exactly one observation, `combine` assigns `math.nan` as the standard deviation, and `json.dumps` emits `NaN`, which strict JSON parsers reject.
Represent an undefined standard deviation with a JSON-valid value, such as `null`, before serializing the result.
Also at `demos/federated-analytics/federated_analytics.py:99`.
</issue_to_address>
### Comment 10
<location path="demos/federated-analytics/federated_analytics.py" line_range="310-315" />
<code_context>
+ submitted = []
+ for idx, part in enumerate(partitions):
+ client = clients[idx % len(clients)]
+ task_id = client.submit(make_task(part, script, image))
+ print(f"Submitted {part['node']} to {client.url}: {task_id}", file=sys.stderr)
+ submitted.append((client, task_id))
+ partials = []
+ for client, task_id in submitted:
+ stdout = client.wait(task_id, **wait_kwargs)
+ try:
+ partials.append(json.loads(stdout.strip().splitlines()[-1]))
</code_context>
<issue_to_address>
**Failed runs leave tasks running**
When a submission or task fails after at least one other task has been submitted, `run_tes` submits every task before waiting; when a later submission or wait fails, it raises without cancelling the other submitted tasks, so the caller exits with an error while those tasks continue consuming service resources.
On failure, cancel outstanding submitted tasks where supported, or otherwise provide an explicit cleanup path and identify the remaining task IDs.
</issue_to_address>
### Comment 11
<location path="demos/federated-analytics/local_stats.py" line_range="51" />
<code_context>
+ agg = per_group.setdefault(col, {"n": 0.0, "sum": 0.0, "sumsq": 0.0})
+ agg["n"] += 1
+ agg["sum"] += value
+ agg["sumsq"] += value * value
+ return stats
+
</code_context>
<issue_to_address>
**Variance loses precision**
When numeric values have a large baseline relative to their spread, the sum-of-squares calculation subtracts two nearly equal large values, so floating-point rounding can reduce a real variance to zero and produce incorrect standard deviations and Welch statistics.
Accumulate and merge numerically stable variance statistics, such as count, mean, and M2, instead of deriving variance from sums and sums of squares.
Also at `demos/federated-analytics/federated_analytics.py:95-97`.
</issue_to_address>
### Comment 12
<location path="demos/federated-analytics/local_stats.py" line_range="43" />
<code_context>
+ for row in rows:
+ try:
+ group = row[GROUP_COLUMN]
+ values = {col: float(row[col]) for col in VALUE_COLUMNS}
+ except (KeyError, TypeError, ValueError) as exc:
+ raise ValueError(f"invalid row {row!r}: {exc}") from exc
</code_context>
<issue_to_address>
**Non-finite measurements pass validation**
When a CSV value is `nan` or `inf`, `local_stats.summarise` accepts `nan` and `inf` because `float()` parses them without raising; aggregation then produces meaningless results, and `--json` can emit non-standard JSON constants.
Reject values for which `math.isfinite()` is false before adding them to the aggregates.
</issue_to_address>Sourcery assessment
Approval pending. 12 findings to address first.
Blocking findings: demos/federated-analytics/federated_analytics.py:221, demos/federated-analytics/federated_analytics.py:182, demos/federated-analytics/federated_analytics.py:273, demos/federated-analytics/federated_analytics.py:243, demos/federated-analytics/local_stats.py:45, and 7 more
Send tokens only over HTTPS, refuse redirects, validate TES responses, cancel sent tasks on failure, emit strict JSON, and reject empty partitions and non-finite values with errors that name no data values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adds the demo
demos/federated-analytics: the same analysis at two Nodes through GA4GH TES, directly or through a proTES gateway, with the results combined. Only aggregates leave the Nodes, never the rows.local_stats.py(stdlib only) runs inside each task and prints the count, sum and sum of squares per condition and column.federated_analytics.pysends one TES task per partition ofdata/manifest.json. The script goes in as acontentinput and the partition as a URL input. The client reads the result from the executor's stdout in the task logs, so no object store is needed. It combines the results into mean, SD and Welch's t.--tes URLonce sends everything through a gateway; once per partition sends each partition to its own Node.TES_TOKENis sent as a bearer token.--localruns without any services.Closes #5
Verification
uv run pytest: 59 passed, 100% coverageuv run ruff check .,uv run ruff format --check .anduv run mypy: cleanpython demos/federated-analytics/federated_analytics.py --local: prints the table withWelch's t (treated vs control), od600: 13.81andexpression: 8.48Lint,TestandCI passedgreenReview
adversarial-reviewerandquality-reviewer, 2 rounds.--tescount must be 1 or the number of partitions; new--polland--timeoutoptions; the deadline uses a monotonic clock that includes request time; HTTP and network errors become a message and exit code 1; the test injects a transport instead of monkeypatching; the ANN401 exception is limited to the demo and its tests.--polland--timeoutmust be positive and finite; the count test is stricter.--jsonis strict JSON (nullinstead ofNaN); empty partitions and non-finite values are rejected;local_stats.pyerrors name the row and column only, never values, so no data leak through task logs.Declined review findings
_http(the real network call) has no test: it is a thinurlopenwrapper, and everything around it is tested through the transport.--timeout; revision-pinned URLs and checksum checks inside the tasks; staged data generation; Welford-style variance (count, mean, M2), a good follow-up if the demo grows.Note for the maintainer
elixir-cloud-aai/docsrepo. An issue for that is drafted and will be posted separately.🤖 Generated with Claude Code