Skip to content

A numpy value in a node's attributes changes its identity: one hashing seam, not a conversion per site #62

Description

@vincenzoml

Answers question 1 of the review of fix/disk-reserve-collapse (b0b68a1, the
1000-epoch training lost to Object of type int64 is not JSON serializable).

Short answer: yes, one seam, and JSON is the smaller half of the problem. A
numpy value in a node's attributes changes that node's identity today.

Measured

lazy/hash.py::hash_node is the only thing that assigns node identity. Feeding
it the same number in two spellings, numpy 2.3.2:

value in attrs["value"] node id (first 16) same as plain Python?
2 9ed0833f3c4356a5
np.int64(2) c1cfd980ec74ebfb no
2.5 fd9930e96c3c9e35
np.float64(2.5) 0678000cf8ee2487 no
np.bool_(True) b06e699e4e3b46d7 no
two distinct bare object()s differ from each other no

Three separate mechanisms, all in _feed:

  1. np.int64 and np.bool_ are not int/bool subclasses, so they fall to
    the final else and are hashed as str(value) under the tag o instead of
    i/T.
  2. np.float64 is a float subclass, so it takes the float branch — and
    that branch uses repr(). In numpy 2 repr(np.float64(2.5)) is
    np.float64(2.5); in numpy 1 it was 2.5. So this one is silent, and it is
    also literally the cross-version instability the commit message predicted.
  3. The else branch's str(value) embeds the object address for anything
    without a defined __str__. Two equal-in-meaning objects hash differently,
    and the same object hashes differently on every run: CSE off, warm store
    permanently cold, no error. _decision_from_pickle already defends against
    exactly this with getattr(fn, "__name__", str(fn)).

Why a per-site conversion cannot close it

There is a live path from a kernel's return value into a node id:
engine/expander.py:124 binds each element of a dynamically expanded for to
reducer.py::_create_constant_node, which is attrs={"value": item}, which is
hashed. So for x in <anything an external library produced> puts those values
into the identity of every node in the body. evaluation.py::RewriteContext.constant
is the same path for rewriters. Neither goes near primitives/nnunet/.

What the store does and does not do

storage.py is keyed by node id only — put_success(node_id, value, ...), and
no value hashing anywhere — so the numpy problem does not reach the store
key through the value. It reaches it through attrs. There is also no
put_failure, so a failed run poisons nothing and a retry is clean.

Proposal

Two changes, at two different boundaries, because they answer different
questions:

  1. Identity — lazy/hash.py::_feed. Replace the silent str(value)
    fallback. Handle the scalar protocols explicitly and in this order: an
    object with item() and ndim == 0 becomes its Python scalar; an object
    with __index__ becomes an int; anything else raises. A value that
    cannot be canonically encoded must not silently receive an unstable
    identity — that is a plan-time error with a name, not a slow run to debug
    six weeks later. The float branch must stop using repr() for anything but
    an exact float (use float(value).hex() or take the exact-type path), or
    numpy 3 will move the hashes again.
  2. Serializability — assert, do not convert. materialize.py::save_state
    is json.dumps with no guard, and _plain in b0b68a1 fixes the one caller
    that was known to break it. Keep _plain where it is, but make save_state
    refuse a payload it cannot encode with a message naming the offending key
    path, so the next primitive to put a numpy scalar on a handle fails in a unit
    test instead of after thirteen hours. Defending inside save_state instead
    of at the boundary would be wrong for the reason the commit message gives;
    defending in both places is not redundant, it is a boundary plus an assertion.

Also, on _plain itself: it recurses. The rule in this codebase is that nothing
recurses (handles.py::iter_handles and _rebuild both use an explicit stack,
and say why). Postprocessing kwargs are shallow so it will not blow up today,
but it should use a stack like its neighbours.

Audit of the other boundaries in primitives/nnunet/

Clean, and for a stated reason in each case:

  • find_best_configuration_for — reads inference_information.json and coerces
    every field with str(...). Already the discipline this issue asks for.
  • determine_postprocessing_for — funnels through _decision_from_pickle, so
    b0b68a1 covers both entry points to the pickle.
  • build_model (cases.py:191) — labels is always DEFAULT_LABELS or a
    value read back from JSON; trained_folds comes from range(nfolds);
    dataset_id is int()-ed in allocate_dataset_id.
  • _as_numpy / predict_image — the value is an image payload, handled by
    value_model, not JSON.

One thing that is not numpy but is the same class of defect:
create_predictor puts uuid.uuid4().hex on the predictor handle
(predictor_registry.store). Node ids are structural, so this does not break
CSE — but the handle's stored bytes differ on every run, so a predictor
handle is never byte-identical across runs. Worth knowing before anything
starts comparing stored values.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions