Skip to content

Commit 09150cd

Browse files
committed
Merge main (#63) and reconcile the two read_blank_definition changes
#63 and this branch both rewrote the superclass loop. Resolved by taking both: #63's _definition_to_doc_type (so $NDICALCDOCUMENTPATH resolves, not just $NDIDOCUMENTPATH) and its warning on an unresolvable superclass, plus this branch's flattening of the superclass list and union of depends_on. The unresolvable case now warns AND keeps the entry -- dropping it would silently shorten the list the schema is validated against, trading one quiet failure for another. DID-python #39 and #40 have merged, so both strict xfails XPASSed and are now plain regression tests: removing a file-bearing document, and the matlab_tests create-add-search-read-remove round trip. #40 also cleared apps/calculators/simple_calc and apps/markgarbage/ valid_interval from the known-unvalidatable list, exactly as the gate said it would, and the local Inf widening in the test is replaced by a call to did.validate.loads_matlab_json. Merging #63 also brought its NDIcalc-vis-matlab coverage into view, which CI sees and a bare checkout does not. Three consequences: - #63's test_binarydoc_symmetry builds a data/generic_file document, which cannot be stored blank: generic_file.json seeds "" for two double fields whose schema allows no empty value. Worked around in the fixture, as in test_cloud_filehandler, rather than patched in the shared JSON. - Two NDIcalc definitions fail their own schemas -- hartley_reverse_ correlation inherits ngrid its schema omits (named by NDI-matlab's own sweep in 854a658), and reverse_correlation seeds an out-of-range integer. Listed separately from NDI's own tree, since they are absent until ndi_install.py runs and a bare checkout must not fail for that. - stimloopsplitter_calc_schema.json is missing the opening quote of a value and is unparseable at NDIcalc's only tag and at its head, so no pin bump fixes it. Quarantined, with the test failing if it ever parses. Verified with the NDIcalc trees installed as CI installs them, against did 8e6f914 (main, carrying both fixes): 1614 passed, 216 skipped, 0 xfail. black and ruff clean; ndi check 18/18. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018gQMtmEgVKNUhvWakNLPLv
2 parents d68ee03 + 8a5d0f7 commit 09150cd

8 files changed

Lines changed: 433 additions & 81 deletions

File tree

‎src/ndi/document.py‎

Lines changed: 71 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
"""
1313

1414
import json
15+
import warnings
1516
from copy import deepcopy
1617
from datetime import datetime, timezone
1718
from pathlib import Path
@@ -33,6 +34,26 @@
3334
from .ido import ndi_ido
3435

3536

37+
def _definition_to_doc_type(definition: str) -> str:
38+
"""Turn a superclass ``definition`` path into a document type name.
39+
40+
Definitions name their target through a path VARIABLE:
41+
``$NDIDOCUMENTPATH/element.json`` for NDI's own documents,
42+
``$NDICALCDOCUMENTPATH/vision/contrast_tuning.json`` for ones shipped
43+
by NDIcalc-vis-matlab. MATLAB registers both as DID globals
44+
(ndi.common.PathConstants), so both resolve there.
45+
46+
Here they resolve to the same folder, because ndi_install.py copies
47+
each dependency's ``ndi_common`` tree into NDI-python's own. So every
48+
variable is stripped rather than only ``$NDIDOCUMENTPATH`` -- which is
49+
what previously left ``$NDICALCDOCUMENTPATH/...`` as a literal path,
50+
unfindable, and silently skipped.
51+
"""
52+
import re
53+
54+
return re.sub(r"^\$[A-Z]+PATH/", "", definition).replace(".json", "")
55+
56+
3657
class ndi_document:
3758
"""NDI document class for database storage.
3859
@@ -496,9 +517,7 @@ def doc_superclass(self) -> list[str]:
496517
definition = sc.get("definition", "")
497518
if definition:
498519
try:
499-
sc_doc = ndi_document(
500-
definition.replace("$NDIDOCUMENTPATH/", "").replace(".json", "")
501-
)
520+
sc_doc = ndi_document(_definition_to_doc_type(definition))
502521
sc_names.append(sc_doc.doc_class())
503522
except Exception:
504523
pass
@@ -771,11 +790,33 @@ def read_blank_definition(document_type: str) -> dict:
771790
Returns:
772791
Dictionary with blank document structure.
773792
"""
774-
# Try to find the JSON definition
775-
json_path = ndi_common_PathConstants.DOCUMENT_PATH / f"{document_type}.json"
793+
root = ndi_common_PathConstants.DOCUMENT_PATH
794+
json_path = root / f"{document_type}.json"
795+
796+
if not json_path.exists() and "/" not in document_type:
797+
# Fall back to a recursive search for a BARE class name.
798+
#
799+
# MATLAB resolves a bare name against the whole document tree,
800+
# so ndi.document('image') finds data/image.json. Here only the
801+
# top level was tried, so the same call needed 'data/image' --
802+
# a difference affecting every document in a subdirectory
803+
# (image, generic_file, the whole stimulus family) and one that
804+
# made otherwise-symmetric code diverge at the call site.
805+
#
806+
# Only for a bare name: a caller who wrote 'data/image'
807+
# expects that exact file, and should not silently get a
808+
# same-named document from elsewhere in the tree.
809+
matches = sorted(root.rglob(f"{document_type}.json"))
810+
if len(matches) == 1:
811+
json_path = matches[0]
812+
elif len(matches) > 1:
813+
rel = ", ".join(str(m.relative_to(root)) for m in matches)
814+
raise FileNotFoundError(
815+
f"ndi_document type {document_type!r} is ambiguous: {rel}. "
816+
f"Give the path, e.g. {matches[0].relative_to(root).with_suffix('')}."
817+
)
776818

777819
if not json_path.exists():
778-
# Try without path constants (for testing)
779820
raise FileNotFoundError(
780821
f"ndi_document definition not found: {json_path}. "
781822
f"Make sure NDI is properly installed."
@@ -808,19 +849,36 @@ def read_blank_definition(document_type: str) -> dict:
808849
continue
809850

810851
entry = dict(sc) if isinstance(sc, dict) else {"definition": sc_def}
852+
merged_superclasses.append(entry)
811853

812-
# Extract document type from definition path
813-
sc_type = sc_def.replace("$NDIDOCUMENTPATH/", "").replace(".json", "")
854+
# Strip whatever $...PATH/ prefix the definition carries, not
855+
# just $NDIDOCUMENTPATH: calculator documents name their
856+
# superclasses through $NDICALCDOCUMENTPATH, and ndi_install.py
857+
# merges every dependency's ndi_common into one tree.
858+
sc_type = _definition_to_doc_type(sc_def)
814859
try:
815860
sc_props = ndi_document.read_blank_definition(sc_type)
816861
except FileNotFoundError:
817-
# Skip missing superclass definitions, but keep the entry:
818-
# dropping it would silently shorten the superclass list.
819-
merged_superclasses.append(entry)
862+
# Warn rather than swallow. A declared superclass that
863+
# cannot be found is a broken definition, and the document
864+
# silently loses everything that superclass would have
865+
# contributed -- including 'base'. That surfaced far away
866+
# as KeyError: 'base' at construction and took a bisect to
867+
# trace back here. Raising would be better still, but is a
868+
# wider behavioural change than this fix.
869+
#
870+
# The entry stays in the list either way: dropping it would
871+
# silently shorten the superclass list the schema is
872+
# validated against, trading one quiet failure for another.
873+
warnings.warn(
874+
f"superclass {sc_def!r} of {document_type!r} could "
875+
f"not be resolved (looked for {sc_type!r}); the "
876+
f"document will be missing whatever it defines",
877+
RuntimeWarning,
878+
stacklevel=2,
879+
)
820880
continue
821881

822-
merged_superclasses.append(entry)
823-
824882
# The superclass's own superclasses join ours. sc_props is
825883
# already merged, so this is the whole transitive closure.
826884
sc_class = sc_props.get("document_class") or {}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
{
2+
"document_class": {
3+
"definition": "$NDIDOCUMENTPATH\/calculator.json",
4+
"validation": "$NDISCHEMAPATH\/calculator_schema.json",
5+
"class_name": "calculator",
6+
"property_list_name": "calculator",
7+
"class_version": 1,
8+
"superclasses": [
9+
{ "definition": "$NDIDOCUMENTPATH\/base.json" },
10+
{ "definition": "$NDIDOCUMENTPATH\/app.json" }
11+
]
12+
},
13+
"calculator": {
14+
}
15+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"classname": "calculator",
3+
"superclasses": [ "base", "app" ],
4+
"depends_on": [ ],
5+
"calculator": [
6+
]
7+
}

‎src/ndi/session/session_base.py‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,29 @@ def empty_id() -> str:
4040
return "".join("0" if c != "_" else "_" for c in base_id)
4141

4242

43+
def _binary_handle(file_path):
44+
"""Open a binary document file with MATLAB's attribute name on it.
45+
46+
MATLAB's ``database_openbinarydoc`` returns an ``ndi.database.binarydoc``
47+
carrying ``fullpathfilename`` -- session.m uses it itself, as the key
48+
for its autoclose listeners. Python returned a bare file object, so
49+
code written against the documented MATLAB API broke here even though
50+
the information was present the whole time under a different name
51+
(``.name``).
52+
53+
The returned object is still an ordinary file object, so every existing
54+
caller is unaffected; it simply also answers to the MATLAB name.
55+
56+
Not yet symmetric: MATLAB takes an ``autoClose`` option implemented
57+
with object-destruction listeners. The Python idiom would be a context
58+
manager rather than a listener, which is a design decision rather than
59+
an attribute, so it is left for its own change.
60+
"""
61+
fh = open(file_path, "rb")
62+
fh.fullpathfilename = str(file_path)
63+
return fh
64+
65+
4366
class ndi_session(ABC):
4467
"""
4568
Abstract base class for NDI sessions.
@@ -504,15 +527,15 @@ def database_openbinarydoc(
504527
if not file_path.exists():
505528
# Attempt on-demand fetch from cloud via ndic:// protocol
506529
if self._try_cloud_fetch(doc, filename, file_path):
507-
return open(file_path, "rb")
530+
return _binary_handle(file_path)
508531
raise FileNotFoundError(
509532
f"Binary file '{filename}' not found for document {doc_id}. "
510533
f"If this is a cloud dataset, ensure NDI_CLOUD_USERNAME and "
511534
f"NDI_CLOUD_PASSWORD environment variables are set, or pass "
512535
f"a cloud_client to the session/dataset."
513536
)
514537

515-
return open(file_path, "rb")
538+
return _binary_handle(file_path)
516539

517540
def database_existbinarydoc(
518541
self,

‎tests/matlab_tests/test_database.py‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,6 @@
2828
class TestNDIDocument:
2929
"""Test full document create-add-search-read-binary workflow."""
3030

31-
@pytest.mark.xfail(
32-
strict=True,
33-
reason=(
34-
"Upstream DID-python bug: sqlitedb's new 'files' table has "
35-
"FOREIGN KEY(doc_idx) REFERENCES docs(doc_idx), but _do_remove_doc "
36-
"never deletes from it, so database_rm on a document carrying a "
37-
"binary raises sqlite3.IntegrityError. No NDI-side fix exists; "
38-
"fixed upstream in VH-Lab/DID-python#39. "
39-
"Tracked, with a minimal repro, in "
40-
"tests/test_did_integration.py::test_removing_a_file_bearing_document "
41-
"and docs/developer_notes/DEPENDENCY_DRIFT_2026-08.md."
42-
),
43-
)
4431
def test_document_creation_and_io(self, tmp_path):
4532
"""Full lifecycle: create → add → search → read binary → remove.
4633

‎tests/test_binarydoc_symmetry.py‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
"""database_openbinarydoc must answer to the MATLAB attribute name.
2+
3+
MATLAB's returns an ndi.database.binarydoc carrying ``fullpathfilename``
4+
-- session.m uses it itself, as the key for its autoclose listeners.
5+
Python returned a bare file object, so code written against the documented
6+
MATLAB API broke here even though the path was present all along under a
7+
different name.
8+
"""
9+
10+
from __future__ import annotations
11+
12+
import pytest
13+
14+
from ndi.document import ndi_document
15+
from ndi.session.dir import ndi_session_dir
16+
17+
PAYLOAD = b"binary payload for the symmetry test\x00\xff"
18+
19+
20+
@pytest.fixture
21+
def session_with_binary(tmp_path):
22+
d = tmp_path / "sess"
23+
d.mkdir(parents=True, exist_ok=True)
24+
S = ndi_session_dir("bin_test", str(d))
25+
26+
payload = tmp_path / "payload.bin"
27+
payload.write_bytes(PAYLOAD)
28+
29+
doc = ndi_document("data/generic_file") + S.newdocument()
30+
# generic_file.json ships "" for two double fields whose schema allows no
31+
# empty value (parameters [0,10000000,1] -- three entries, so can-be-empty
32+
# is off), so a blank generic_file document cannot be added in either
33+
# language. The defect is in the shared ndi_common JSON and is reported
34+
# upstream; worked around here rather than patched in NDI-python's copy.
35+
# See docs/developer_notes/DEPENDENCY_DRIFT_2026-08.md.
36+
doc.document_properties["generic_file"]["dateCreated"] = 0
37+
doc.document_properties["generic_file"]["dateUpdated"] = 0
38+
doc = doc.add_file("generic_file.ext", str(payload))
39+
S.database_add(doc)
40+
return S, doc, "generic_file.ext"
41+
42+
43+
def test_handle_carries_fullpathfilename(session_with_binary):
44+
S, doc, name = session_with_binary
45+
fh = S.database_openbinarydoc(doc, name)
46+
try:
47+
assert hasattr(fh, "fullpathfilename"), (
48+
"MATLAB's binarydoc exposes fullpathfilename; code written "
49+
"against that API must work here too"
50+
)
51+
assert fh.fullpathfilename == fh.name
52+
assert fh.fullpathfilename.endswith(name)
53+
finally:
54+
S.database_closebinarydoc(fh)
55+
56+
57+
def test_handle_is_still_an_ordinary_file_object(session_with_binary):
58+
"""The attribute is additive. Every existing caller reads bytes from
59+
this object and must be unaffected."""
60+
S, doc, name = session_with_binary
61+
fh = S.database_openbinarydoc(doc, name)
62+
try:
63+
assert fh.read() == PAYLOAD
64+
finally:
65+
S.database_closebinarydoc(fh)
66+
67+
68+
def test_path_can_be_reopened_by_name(session_with_binary):
69+
"""The whole point: a caller can hand the path to something that
70+
wants a filename rather than a handle."""
71+
S, doc, name = session_with_binary
72+
fh = S.database_openbinarydoc(doc, name)
73+
path = fh.fullpathfilename
74+
S.database_closebinarydoc(fh)
75+
# Reopened by path alone, with no handle and no session involved.
76+
with open(path, "rb") as f2:
77+
assert f2.read() == PAYLOAD

0 commit comments

Comments
 (0)