Skip to content

Operations discard externally-set instance attributes, breaking oops backplane keys #18

Description

@rfrenchseti

Summary

As of 1.0.1.dev63 (62713f0, 63 commits past v1.0.0), most operations that return a new Qube silently discard instance attributes that were set on the source object from outside polymath. Under rms-polymath 1.0.0 those attributes survived. This breaks rms-oops, which attaches a .key attribute to every registered backplane object, and through it breaks ring navigation in SpinDoctor.

The failure is silent rather than loud, which is what makes it critical: the resulting AttributeError is raised deep inside a model build, callers that catch broad exceptions swallow it, and the pipeline carries on and reports success with a degraded answer. In a 22-image navigation benchmark, 5 images lost a ring technique and only 1 of those 5 changed its reported status; the other 4 still reported status=success with a plausible offset computed from the techniques that survived.

Reproduction

import numpy as np
from polymath import Scalar

s = Scalar(np.arange(12.).reshape(3, 4), mask=np.zeros((3, 4), bool))
s.key = 'K'

print(getattr(s.copy(),          'key', 'LOST'))
print(getattr(s.clone(),         'key', 'LOST'))
print(getattr(s + 0,             'key', 'LOST'))
print(getattr(s.mask_where_eq(3.), 'key', 'LOST'))
print(getattr(s.collapse_mask(), 'key', 'LOST'))
print(getattr(s.remask_or(np.zeros((3, 4), bool)), 'key', 'LOST'))

Under 1.0.0 every line prints K. Under 1.0.1.dev63 every line prints LOST.

Operation-by-operation, comparing the two versions:

Operation 1.0.0 1.0.1.dev63
copy() preserved dropped
clone() preserved dropped
arithmetic (s + 0) preserved dropped
mask_where_eq() preserved dropped
collapse_mask() preserved dropped
remask_or() preserved dropped
.wod preserved preserved
as_readonly() preserved preserved
indexing (s[0:5]) dropped dropped

Indexing already dropped the attribute in 1.0.0, so that one is not a regression. The other six are.

Root cause

Qube.clone() in 1.0.0 copied the instance dictionary wholesale, so any externally-set attribute came along:

for attr, value in self.__dict__.items():
    if attr in ('_derivs', '_cache'):
        obj.__dict__[attr] = {}
    ...

Qube.clone() now delegates to Qube._transfer_attrs() (src/polymath/qube.py:431-450), which copies a fixed allowlist by name:

_TRANSFERABLE_ATTRS = ('_values', '_mask', '_is_array', '_is_scalar', '_shape',
                       '_ndims', '_rank', '_nrank', '_drank', '_item', '_numer',
                       '_denom', '_size', '_isize', '_nsize', '_dsize', '_unit',
                       '_readonly', '_truth_if_any', '_truth_if_all', '_default')

_OPTIONAL_ATTRS = ('_pickle_digits', '_pickle_reference')

The allowlist enumerates polymath's own attributes, so nothing a downstream package attached can be carried. The docstring states the rationale — naming the attributes avoids materializing __dict__ and forfeiting CPython's inline attribute storage — so this is a deliberate optimization whose cost to external attributes appears to have been unintended. The same allowlist reaches the other affected operations through _new_from_parts() and the fast result constructors introduced in the same performance series (52b3475, 3ae8699, 915939c).

Downstream impact

rms-oops sets the attribute in Backplane.register_backplane() (oops/backplane/__init__.py:902):

# For reference, we add the key as an attribute of each backplane object
backplane.key = key

Under 1.0.1.dev63 the attribute does not survive to the caller. Reproduced in situ against real holdings on a Cassini ISS ring image:

obs = ObsCassiniISS.from_file(FCPath('.../N1863267861_1_CALIB.IMG'))
bp = obs.ext_bp.ring_radius('saturn:ring')
hasattr(bp, 'key')
# 1.0.0        -> True,  key = ('ring_radius', ('SUN<', 'SATURN:RING'), None, None)
# 1.0.1.dev63  -> False

In SpinDoctor this surfaces as AttributeError: 'Scalar' object has no attribute 'key' raised from NavModelRings, where the ring-resolvability filter passes the backplane key back to border_atop(). The navigation orchestrator catches per-model exceptions by design, logs them, and continues without that model, so the entire ring model is discarded and every ring technique disappears from the run.

Measured over a 22-image benchmark spanning all 17 library scene classes and all 4 missions, holding everything constant except PYTHONPATH:

  • N1863267861_1_CALIBsuccess -> failed, rank_1_only -> no_features_extracted. RingEdgeNav was its only technique.
  • N1447064164_1_CALIB and N1489603610_2_CALIB — lost RingAnnulusNav, still reported success.
  • N1484688342_1_CALIB and N1598065103_1_CALIB — lost RingEdgeNav, still reported success.

A secondary effect worth flagging for anyone benchmarking this branch: skipping the ring model makes those images finish much faster, which inflates apparent speedup. The five affected images "improved" by 2.35x while the 17 unaffected images improved by 1.19x. The 2.35x is work not being done.

Suggested fix

Any of these would restore the previous contract; the first preserves the optimization for the common case:

  1. Have _transfer_attrs() copy the allowlist as it does now, then additionally copy any keys present in source.__dict__ that are not already handled. Objects that never acquire an external attribute keep their inline storage, because __dict__ is only consulted when it already exists.
  2. Extend _OPTIONAL_ATTRS with a documented mechanism for downstream packages to register attributes that must survive operations.
  3. Declare explicitly that external attributes are not preserved across operations, and fix rms-oops to stop relying on it — for example by having Backplane carry the key in its own dictionary rather than on the array object. This is the largest change and would need a coordinated release.

If option 3 is chosen, note that rms-oops at 0.2.1.dev7 relies on the current behavior today, so shipping 1.0.1 before the oops change lands would break ring navigation for every downstream consumer.

Environment

  • Broken: rms-polymath 1.0.1.dev63 at 62713f0 (v1.0.0-63-g62713f0)
  • Working: rms-polymath 1.0.0 from PyPI
  • rms-oops 0.2.1.dev7, rms-spindoctor 0.1.dev102, Python 3.12.3, NumPy 2.x, Linux

Activity

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

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions