Repository navigation
Conversation
Test CoverageTotal: 73.38% (19729 / 26885 lines)
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| The key is reserved for class and function references, so allowing it in | ||
| user data would make ``to_dict`` output ambiguous. | ||
| """ | ||
| if isinstance(obj, Mapping): |
There was a problem hiding this comment.
[medium] Non-str mapping keys collide with string keys in the id
json.dumps silently converts int, float, bool and None keys to strings, so Evaluation({"x": {1: "a"}}) and Evaluation({"x": {"1": "a"}}) get the same id, compare equal, and are reported as duplicates by validate_evaluations. Evaluation({"x": {True: 1}}) collides with {"true": 1} in the same way. The Evaluation docstring says params are str-keyed mappings, but nothing enforces that. Fix: in the Mapping branch, raise TypeError for any key where not isinstance(key, str), and add a test that {1: ...} is rejected.
| f"{obj!r} is not a module-level class or function and cannot " | ||
| "be serialized by import path." | ||
| ) | ||
| return {"__type__": f"{module}.{qualname}"} |
There was a problem hiding this comment.
[medium] Classes that do not round-trip by import path are accepted
_encode serializes any class or function as module.qualname without checking that the path resolves back to the same object. A class defined in main resolves only in the process that defined it, and a module-level alias made with type() has a qualname that no module attribute matches. In both cases construction succeeds and the id is computed, but Evaluation.from_dict on the stored JSON raises ValueError in a fresh process, which contradicts the design's round-trip guarantee and its claim that bad params fail at construction. Fix: reject module == "main", and check that _resolve(path) is obj, raising TypeError otherwise. Also add tests for both cases.
| for i in range(len(parts), 0, -1): | ||
| prefix = ".".join(parts[:i]) | ||
| try: | ||
| obj = importlib.import_module(prefix) |
There was a problem hiding this comment.
[low] Leading-dot path escapes as TypeError, not ValueError
For a path such as ".json", importlib.import_module(".json") raises TypeError (the package argument is required for relative imports). That is not caught by the except clauses in _resolve, so from_dict raises TypeError although its docstring promises ValueError. Fix: reject paths that start with "." or contain empty parts before the loop. Also, from_dict imports any installed module named in its input, and that import can run side effects. The docstring should say that from_dict must only receive trusted data.
There was a problem hiding this comment.
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:
- Non-string mapping keys are not rejected.
_check_no_type_keydoesn't check key types, andjson.dumpscoerces keys. SoEvaluation({"p": {1: "a"}})andEvaluation({"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 raiseTypeErrorfor non-strkeys anywhere inparams, and add a test. - Some classes get an id but can't be rebuilt.
_encodeaccepts classes and functions whosemodule.qualnamedoesn't resolve back to the same object. Examples are classes defined in__main__and module-level aliases created withtype(). Their ids compute, butfrom_dictcan'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 objand excluding__main__, or document the limitation clearly. Please add a test either way.
Optional suggestions:
- A path with a leading dot (such as
".x") raisesTypeErrorfromimportlibinstead of the documentedValueError. Please catch it in_resolve. from_dictimports any installed module named in its input. A note in the docstring that it is for trusted input only would help.Result.statusisn't validated. I understand this is deferred on purpose, so it's fine to leave.
Reviewed commit 52eb8e7
|
|
||
| status: Literal["success", "failed"] | ||
| metrics: Mapping[str, Any] | ||
| metadata: Mapping[str, Any] = field(default_factory=dict) |
There was a problem hiding this comment.
can you explicitly add an artifact field for this? this is generally useful for saving checkpoints that can be reused in other benchmark.
There was a problem hiding this comment.
added artifact: Path | None to Result, for an evaluation's reusable output, e.g. a checkpoint:
- single field so theres no key naming to agree on
- can point to a folder if a checkpoint is several files/ there are multiple artifacts
This PR adds the base abstraction for a
pyhealth.benchmarkspackage:Benchmark,Evaluation, andResult. 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
Evaluation.idis a SHA-256 of the canonical JSON ofparams(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).paramsas Python objects and are serialized by import path, e.g.{"__type__": "pyhealth.models.rnn.RNN"}. This follows the import-path approach already used bypyhealth.autoimprove.ModelSpec.Evaluation.from_dictresolves 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, sopyhealth.models.RNNandpyhealth.models.rnn.RNNgive the same id."__type__"is a reserved key inparams."__type__"key fail immediately.Result.statusis"success"or"failed"._evaluateis expected to catch per-evaluation failures and return failed Results, so one crash doesn't discard a whole batch. An exception that escapes_evaluatemeans the whole batch failed.evaluate()is final, and it runsvalidate_evaluations → _evaluate → validate_results. Subclasses implement_evaluateand extend the validators viasuper(). The base validators reject duplicate evaluations and require exactly one result per evaluation. Validation reports every problem at once.metadata["error"]must never be published.metadata["error_type"](the exception class name) is the display-safe field.cache_dirmay be shared by concurrent jobs, so benchmarks must write atomically (temporary file, thenos.replace). Benchmarks own their cache keys and reuse decisions.base.pycontains the public interface and its contracts._serialization.pyholds 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
paramsmust not be mutated after construction, because the id is cached and is not deep-frozen.1and1.0give different ids.Deferred (add when needed)
versionproperty (non-abstract, with a default)group_evaluations()scheduling hintValueErrorsparamsResulthelper constructors and validationFiles to review
pyhealth/benchmarks/base.py: the public interface (Benchmark,Evaluation,Result) and their contractspyhealth/benchmarks/_serialization.py: private helpers for canonical JSON, encoding import paths, and resolving thempyhealth/benchmarks/__init__.py: exportstests/core/test_benchmark_base.py: tests (unittest, so they run under bothpytestandpixi run test), including thefrom_dictround trip and a golden-id test that guards the canonical serializationdocs/api/benchmarks.rstanddocs/index.rst: API docs page and toctree entry