[fbonc]

This PR adds the base abstraction for a pyhealth.benchmarks package: Benchmark, Evaluation, and Result. Concrete benchmarks (starting with CPBench) will subclass it.

It is deliberately minimal. It contains only what is needed to define, validate, and execute a batch of evaluations. Anything consumed only by the job runner, result storage, or leaderboard is left out until those components exist. The "Deferred" section below lists each omission and what would trigger adding it. Every item there can be added later without breaking existing subclasses.

Design decisions

• The base class is agnostic to PyHealth internals. It knows how to identify, validate, and execute evaluations. It knows nothing about datasets, models, training, conformal methods, or metrics. Execution logic, caching, and optimizations all belong to individual benchmarks.
• A benchmark is not a runner or a leaderboard. It defines evaluations and executes them. Deciding which evaluations to run, when to run them, and how to publish results is left to external infrastructure.
• Evaluation identity is defined by its params. Evaluation.id is a SHA-256 of the canonical JSON of params (sorted keys). Evaluations are compared and hashed by id, so they work as dict keys and storage keys. An id is unique within one benchmark, so storage should key on (benchmark.name, evaluation.id).
• No registries. Classes and functions appear in params as Python objects and are serialized by import path, e.g. {"__type__": "pyhealth.models.rnn.RNN"}. This follows the import-path approach already used by pyhealth.autoimprove.ModelSpec. Evaluation.from_dict resolves these paths back to objects, so evaluations round-trip through JSON and YAML. Public re-export paths are accepted, and the id always uses the defining module, so pyhealth.models.RNN and pyhealth.models.rnn.RNN give the same id. "__type__" is a reserved key in params.
• Params are validated at construction. Computing the id serializes the params, so non-JSON values, NaN, lambdas, locally defined classes, and the reserved "__type__" key fail immediately.
• Partial failure is supported. Result.status is "success" or "failed". _evaluate is expected to catch per-evaluation failures and return failed Results, so one crash doesn't discard a whole batch. An exception that escapes _evaluate means the whole batch failed.
• Validation is guaranteed. evaluate() is final, and it runs validate_evaluations → _evaluate → validate_results. Subclasses implement _evaluate and extend the validators via super(). The base validators reject duplicate evaluations and require exactly one result per evaluation. Validation reports every problem at once.
• Error text is private. Exception messages and tracebacks can quote patient data, so metadata["error"] must never be published. metadata["error_type"] (the exception class name) is the display-safe field.
• Cache contract. cache_dir may be shared by concurrent jobs, so benchmarks must write atomically (temporary file, then os.replace). Benchmarks own their cache keys and reuse decisions.
• The interface is kept separate from the serialization machinery. base.py contains the public interface and its contracts. _serialization.py holds the private helpers for canonical JSON, encoding import paths, and resolving them. All helpers are _-prefixed and tested only through the public API, so they can be refactored freely.

Known trade-offs

• params must not be mutated after construction, because the id is cached and is not deep-frozen.
• Moving a referenced class to another module changes the id, even when its public import path is unchanged.
• No numeric normalization: 1 and 1.0 give different ids.

Deferred (add when needed)

Not included	Add when
version property (non-abstract, with a default)	The runner reuses stored results and needs to detect stale ones
group_evaluations() scheduling hint	The runner splits batches across cluster jobs, and evaluations sharing a checkpoint should run together
Metric metadata (direction, coverage targets)	The leaderboard needs to sort and highlight metrics. Coverage targets depend on α, so they likely belong per evaluation, not per benchmark
Custom exception types	A caller needs to tell validation failures apart from other ValueErrors
Deep-freezing params	Mutation bugs actually show up
Result helper constructors and validation	Benchmarks drift into inconsistent failure shapes

Files to review

• pyhealth/benchmarks/base.py: the public interface (Benchmark, Evaluation, Result) and their contracts
• pyhealth/benchmarks/_serialization.py: private helpers for canonical JSON, encoding import paths, and resolving them
• pyhealth/benchmarks/__init__.py: exports
• tests/core/test_benchmark_base.py: tests (unittest, so they run under both pytest and pixi run test), including the from_dict round trip and a golden-id test that guards the canonical serialization
• docs/api/benchmarks.rst and docs/index.rst: API docs page and toctree entry

[github-actions]

Test Coverage

Total: 73.38% (19729 / 26885 lines)

Modified file	Coverage	Lines
pyhealth/benchmarks/__init__.py	100.00%	2 / 2
pyhealth/benchmarks/_serialization.py	95.95%	71 / 74
pyhealth/benchmarks/base.py	98.33%	59 / 60
pyhealth/calib/calibration/__init__.py	100.00%	6 / 6
pyhealth/calib/calibration/hb.py	75.00%	84 / 112
pyhealth/calib/calibration/logistic_recalibration.py	97.47%	77 / 79
pyhealth/calib/predictionset/__init__.py	100.00%	9 / 9
pyhealth/calib/predictionset/base_conformal/__init__.py	62.89%	61 / 97
pyhealth/calib/predictionset/favmac/core.py	18.58%	21 / 113
pyhealth/calib/predictionset/label_shift/__init__.py	100.00%	4 / 4
pyhealth/calib/predictionset/label_shift/black_box_shift.py	87.69%	57 / 65
pyhealth/calib/predictionset/label_shift/label_shift.py	96.39%	80 / 83
pyhealth/calib/predictionset/label_shift/mlls.py	95.12%	78 / 82
pyhealth/calib/predictionset/rc3p.py	96.06%	122 / 127
pyhealth/calib/predictionset/scrib/quicksearch.py	79.10%	193 / 244
pyhealth/datasets/__init__.py	83.33%	40 / 48
pyhealth/datasets/base_dataset.py	90.84%	476 / 524
pyhealth/datasets/ptbxl.py	83.51%	157 / 188
pyhealth/datasets/sample_dataset.py	94.41%	321 / 340
pyhealth/datasets/splitter.py	96.62%	343 / 355
pyhealth/metrics/__init__.py	100.00%	11 / 11
pyhealth/metrics/binary.py	75.00%	63 / 84
pyhealth/metrics/multiclass.py	100.00%	116 / 116
pyhealth/metrics/multilabel.py	96.52%	111 / 115
pyhealth/metrics/regression.py	55.56%	20 / 36
pyhealth/models/base_model.py	93.01%	133 / 143
pyhealth/models/rnn.py	89.18%	173 / 194
pyhealth/models/transformers_model.py	90.00%	81 / 90
pyhealth/processors/__init__.py	94.12%	32 / 34
pyhealth/processors/temporal_timeseries_processor.py	78.57%	44 / 56
pyhealth/processors/tensor_processor.py	92.50%	74 / 80
pyhealth/processors/timeseries_processor.py	84.48%	98 / 116
pyhealth/processors/utils.py	96.55%	56 / 58
pyhealth/tasks/__init__.py	89.19%	33 / 37
pyhealth/tasks/ptbxl.py	93.42%	71 / 76
pyhealth/tasks/sleep_staging.py	93.33%	84 / 90
pyhealth/trainer.py	72.69%	197 / 271

Workflow run · adceb6e

[Logiquo]

/review

[github-actions]

Previous review

_{Reviewed commit 52eb8e7}

[github-actions]

Previous review

AI Review

suggestion: need change
decision: human review

Thank you for this PR. It adds a minimal pyhealth.benchmarks base layer (Benchmark, Evaluation, Result), a private canonical-serialization module, API docs and tests. The design is clear and well documented, and the validation flow is sensible. Coverage is above the bar: base.py is 98.31%, _serialization.py is 95.16%, __init__.py is 100%, and the whole repository is 70.28% against the 60% bar.

To get this merged:

1. Non-string mapping keys are not rejected. _check_no_type_key doesn't check key types, and json.dumps coerces keys. So Evaluation({"p": {1: "a"}}) and Evaluation({"p": {"1": "a"}}) get the same id and compare equal. That contradicts the documented "str-keyed mappings" contract and could make two different evaluations collide in storage. Please raise TypeError for non-str keys anywhere in params, and add a test.
2. Some classes get an id but can't be rebuilt. _encode accepts classes and functions whose module.qualname doesn't resolve back to the same object. Examples are classes defined in __main__ and module-level aliases created with type(). Their ids compute, but from_dict can't rebuild them in another process. That breaks the stated round-trip guarantee. Please either reject such objects at construction, for example by checking that _resolve(path) is obj and excluding __main__, or document the limitation clearly. Please add a test either way.

Optional suggestions:

• A path with a leading dot (such as ".x") raises TypeError from importlib instead of the documented ValueError. Please catch it in _resolve.
• from_dict imports any installed module named in its input. A note in the docstring that it is for trusted input only would help.
• Result.status isn't validated. I understand this is deferred on purpose, so it's fine to leave.

_{Reviewed commit 52eb8e7}

[fbonc]

/review

[Logiquo]

/review

[github-actions]

_{Reviewed commit adceb6e}

[github-actions]

[medium] from_dict imports arbitrary modules and returns any function named in its input

_resolve calls importlib.import_module on every dotted path found in __type__ and then getattrs the rest. _is_type_ref (line 145) accepts any plain or builtin function, so input such as {"__type__": "os.system"} resolves successfully. Evaluation.from_dict is positioned as the loader for JSON/YAML specs that the future runner will read, so loading a spec from an untrusted or shared source imports any installed module (running its top-level code) and returns callables that the runner may later call with the params. The PR does not state this trust boundary. Fix: document in Evaluation.from_dict that its input must be trusted, and either restrict resolution to an allowlist of module prefixes (for example pyhealth. and the benchmark's own modules) or limit it to classes, with a test that a disallowed path is rejected.

[github-actions]

[low] Relative-looking type paths raise TypeError, not the documented ValueError

importlib.import_module raises TypeError for names with a leading dot when no package is given, for example ".json" or ".". Only ValueError is caught here, so Evaluation.from_dict({"c": {"__type__": ".json"}}) raises TypeError, while from_dict documents ValueError for unresolvable paths. Fix: reject paths that have an empty component, or catch (ValueError, TypeError), and add ".json" to test_unresolvable_path_raises.

[github-actions]

AI Review

suggestion: need change
decision: human review

Thank you for this contribution. The minimal Benchmark, Evaluation and Result abstraction is clearly thought through. I like the canonical-JSON ids, the import-path encoding, and the way the deferred items are spelled out. Tests are thorough, and coverage is good: the new files are at 96–100%, and the repository total of 73.4% is above the 60% bar. The change looks close to ready.

To get this merged:

• Document or restrict what Evaluation.from_dict will load. _resolve in _serialization.py imports any module named in the input and returns any callable it finds there, including functions such as os.system. It doesn't call them, but importing a module can run code, and a benchmark may later call whatever a loaded params entry points to. Please do one of these:
  • State in the from_dict docstring, and in the docs page, that it must only be given trusted input.
  • Or restrict resolution, for example to an allow-list of module prefixes, or to a caller-supplied check.

Optional suggestions:

• Relative-looking paths such as ".json" raise TypeError instead of the documented ValueError. Catching that case in _resolve would make the contract match.
• Tests for the super() validator hooks, the "reports every problem at once" claim, and cache_dir pass-through would round out the suite.

_{Reviewed commit adceb6e}

[Logiquo]

• Document or restrict what Evaluation.from_dict will load. _resolve in _serialization.py imports any module named in the input and returns any callable it finds there, including functions such as os.system. It doesn't call them, but importing a module can run code, and a benchmark may later call whatever a loaded params entry points to. Please do one of these:

  • State in the from_dict docstring, and in the docs page, that it must only be given trusted input.
  • Or restrict resolution, for example to an allow-list of module prefixes, or to a caller-supplied check.

I think security check can be left as the subclass to handle it.