Housekeeping ahead of the Inc 1-4 coverage work
- inventory.py --methods: report the instance methods the app calls per class, plus chained calls that constrain a return type. --check says which classes to bind; this says what to bind on them, which is what writing ~38 module TUs needs. - Drop StlAPI from the inventory: StlAPI_Writer has no app call site (the only use was a test fixture, now on the app's own STL writer). 138 symbols / 47 modules. - adding-symbols.md: scope the executing-constructor ban to the BRepAlgoAPI booleans, which are the only classes with a deferred Set*/Build form — BRepMesh_IncrementalMesh, GeomAPI_*, BRepCheck_Analyzer and friends compute in their constructor by design and bind as stock. Replace the per-increment app-test guidance: backend/tests/conftest.py imports n3xd.main, so no app test can collect until the last module is bound. Increments gate on stock-recorded fixtures here; the app suite is the Inc 4 gate. - parity_venv.sh: run the ocp suite in the swapped venv (it imports only OCP/n3xd_ocp, so it works throughout). - Fix a stale macro name in occt_handle.h (ocp_new, not OCP_TRANSIENT_NEW) and drop the unused ocp_transient_class helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfriM8XUkn7uYf5Dwe2xo6
This commit is contained in:
@@ -6,15 +6,22 @@ Read [design.md](design.md) first if you are touching the machinery instead.
|
|||||||
## 1. Find what is missing
|
## 1. Find what is missing
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
python tools/inventory.py --emit # re-scan the app for OCP usage
|
python tools/inventory.py --emit # re-scan the app for OCP usage
|
||||||
python tools/inventory.py --check # what the installed wheel lacks, by module
|
python tools/inventory.py --check # what the wheel lacks, by module
|
||||||
|
python tools/inventory.py --methods --only BRepAdaptor # what to bind on each class
|
||||||
```
|
```
|
||||||
|
|
||||||
`--check` groups gaps by module, which is how increments are scoped. Note it
|
`--check` groups gaps by module, which is how increments are scoped. It only
|
||||||
only sees symbols reached through an import (`TopExp.MapShapes_s`), not methods
|
sees symbols reached through an import (`TopExp.MapShapes_s`), so it answers
|
||||||
called on instances (`shape.IsSame(...)`) — a green `--check` is necessary, not
|
*which* classes to bind but not *what* to bind on them.
|
||||||
sufficient. Running the app's own tests in the parity venv is what catches the
|
|
||||||
rest, loudly, as an `AttributeError`.
|
`--methods` answers the second question: it resolves variables assigned
|
||||||
|
straight from a constructor and reports the methods called on them, plus
|
||||||
|
chained calls as `Klass.Outer() -> Inner` (those constrain `Outer`'s **return**
|
||||||
|
type — `adaptor.Cylinder().Radius()` is a requirement on `gp_Cylinder`). It is
|
||||||
|
a heuristic — it does not follow arguments, returns or attributes, and a local
|
||||||
|
reassigned from something else shows up as noise — so read it as a starting
|
||||||
|
surface, not a specification.
|
||||||
|
|
||||||
## 2. Write the module
|
## 2. Write the module
|
||||||
|
|
||||||
@@ -50,8 +57,13 @@ only in that a base class must precede its derived classes.
|
|||||||
`nb::class_` declaration and bind constructors with `ocp_new<T, Args...>()`.
|
`nb::class_` declaration and bind constructors with `ocp_new<T, Args...>()`.
|
||||||
Never `nb::init<>` — see design.md.
|
Never `nb::init<>` — see design.md.
|
||||||
- **Long kernel call** → `OCP_NOGIL`, but only if it cannot re-enter Python.
|
- **Long kernel call** → `OCP_NOGIL`, but only if it cannot re-enter Python.
|
||||||
- **Executing constructor** → do not bind it. Bind the default constructor plus
|
- **Executing constructor** → banned only where a deferred `SetX`/`Build` API
|
||||||
the `SetX`/`Build` sequence.
|
exists and the constructor duplicates `Build()`: the `BRepAlgoAPI_*` booleans
|
||||||
|
and splitter. Bind their default constructor plus the `SetX`/`Build`
|
||||||
|
sequence. Classes that only compute in their constructor and have no deferred
|
||||||
|
form — `BRepMesh_IncrementalMesh`, `BRepCheck_Analyzer`, `GeomAPI_*`,
|
||||||
|
`BRepClass3d_SolidClassifier`, `BRepExtrema_DistShapeShape`, `GCPnts_*`,
|
||||||
|
`BRepBuilderAPI_Transform` — bind exactly as stock does.
|
||||||
- **Enum** → `nb::is_arithmetic()` and `.export_values()`.
|
- **Enum** → `nb::is_arithmetic()` and `.export_values()`.
|
||||||
- **`Message_ProgressRange` parameters** → omit them. The app never passes one
|
- **`Message_ProgressRange` parameters** → omit them. The app never passes one
|
||||||
(no `OCP.Message` import anywhere), and leaving them out keeps signatures
|
(no `OCP.Message` import anywhere), and leaving them out keeps signatures
|
||||||
@@ -82,11 +94,18 @@ make publish
|
|||||||
tools/parity_venv.sh && python tools/inventory.py --check
|
tools/parity_venv.sh && python tools/inventory.py --check
|
||||||
```
|
```
|
||||||
|
|
||||||
Then run the slice of the app's suite the increment claims — Inc 1 is gated on
|
**The app's own tests cannot gate an individual increment.** `backend/tests/
|
||||||
`backend/tests/test_geom_memo.py`, Inc 2 on the tessellation tests plus
|
conftest.py` imports `n3xd.main`, which pulls in the whole app and therefore the
|
||||||
`pytest -m perf`, Inc 4 on `test_cad_pool.py` and `test_derive.py`, and the
|
whole OCP surface, so every backend test fails at collection until the last
|
||||||
cutover on the full suite plus `backend/tools/rebuild_sweep.py --diff` over the
|
module is bound. Increments are gated here instead: `tools/gen_fixtures.py`
|
||||||
project store.
|
records reference values from the *stock* wheel (counts, `Modified`/`Generated`/
|
||||||
|
`IsDeleted` history maps, measured floats) into `tests/data/manifest.json`, and
|
||||||
|
`tests/test_inc<N>_*.py` reproduces the same constructions under our wheel.
|
||||||
|
Counts and history maps must match exactly; floats compare at rel 1e-9.
|
||||||
|
|
||||||
|
The app's full suite is the **Inc 4** gate, run in the parity venv, alongside
|
||||||
|
`pytest -m perf` and `backend/tools/rebuild_sweep.py --diff` over the project
|
||||||
|
store.
|
||||||
|
|
||||||
## Adding to `n3xd_ocp` instead
|
## Adding to `n3xd_ocp` instead
|
||||||
|
|
||||||
|
|||||||
@@ -212,9 +212,6 @@
|
|||||||
"StdPrs_BRepFont": [],
|
"StdPrs_BRepFont": [],
|
||||||
"StdPrs_BRepTextBuilder": []
|
"StdPrs_BRepTextBuilder": []
|
||||||
},
|
},
|
||||||
"StlAPI": {
|
|
||||||
"StlAPI_Writer": []
|
|
||||||
},
|
|
||||||
"TColStd": {
|
"TColStd": {
|
||||||
"TColStd_Array1OfInteger": [],
|
"TColStd_Array1OfInteger": [],
|
||||||
"TColStd_Array1OfReal": [],
|
"TColStd_Array1OfReal": [],
|
||||||
|
|||||||
@@ -22,7 +22,7 @@
|
|||||||
|
|
||||||
That last sentence is only true if every wrapped transient was heap
|
That last sentence is only true if every wrapped transient was heap
|
||||||
allocated and is handle-owned. See occt_transient.h — transient
|
allocated and is handle-owned. See occt_transient.h — transient
|
||||||
constructors are bound through OCP_TRANSIENT_NEW, never nb::init<>, so
|
constructors are bound through ocp_new<T, Args...>(), never nb::init<>, so
|
||||||
Python never owns transient storage. from_python re-checks the invariant
|
Python never owns transient storage. from_python re-checks the invariant
|
||||||
rather than trusting it, because the failure mode is a double free.
|
rather than trusting it, because the failure mode is a double free.
|
||||||
*/
|
*/
|
||||||
|
|||||||
@@ -24,13 +24,6 @@
|
|||||||
|
|
||||||
namespace nb = nanobind;
|
namespace nb = nanobind;
|
||||||
|
|
||||||
/// Declare a bound transient class. Bind its constructors with ocp_new<T,
|
|
||||||
/// Args...>() — nb::init<> on such a class is a bug (see above).
|
|
||||||
template <typename T, typename... Bases>
|
|
||||||
nb::class_<T, Bases...> ocp_transient_class(nb::handle scope, const char *name) {
|
|
||||||
return nb::class_<T, Bases...>(scope, name);
|
|
||||||
}
|
|
||||||
|
|
||||||
/// Heap-allocating constructor for a transient class:
|
/// Heap-allocating constructor for a transient class:
|
||||||
///
|
///
|
||||||
/// cls.def(ocp_new<Poly_Triangulation, int, int, bool>(),
|
/// cls.def(ocp_new<Poly_Triangulation, int, int, bool>(),
|
||||||
|
|||||||
@@ -80,6 +80,91 @@ def scan(app_root: pathlib.Path) -> dict:
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def methods(app_root: pathlib.Path, only: str | None) -> int:
|
||||||
|
"""Report the *instance* methods the app calls on each OCP class.
|
||||||
|
|
||||||
|
`scan` only sees names reached through an import, which is enough to know
|
||||||
|
*which* classes to bind but not *what* to bind on them. This fills that
|
||||||
|
gap well enough to write a module in one pass instead of discovering the
|
||||||
|
surface one AttributeError at a time: variables assigned straight from a
|
||||||
|
constructor carry their class through the file, so `a = BRepAdaptor_Surface(f)`
|
||||||
|
followed by `a.GetType()` is resolved.
|
||||||
|
|
||||||
|
Chained calls are reported separately as `Klass.Outer() -> Inner`, because
|
||||||
|
what they constrain is the *return* type — `adaptor.Cylinder().Radius()`
|
||||||
|
says gp_Cylinder needs `Radius`, not that BRepAdaptor_Surface does.
|
||||||
|
|
||||||
|
Heuristic by construction: it does not follow arguments, returns or
|
||||||
|
attributes, so treat a quiet class as "look again", not "nothing needed".
|
||||||
|
"""
|
||||||
|
direct: dict[str, set[str]] = collections.defaultdict(set)
|
||||||
|
chained: dict[str, set[str]] = collections.defaultdict(set)
|
||||||
|
|
||||||
|
for path in sorted(app_root.rglob("*.py")):
|
||||||
|
if "__pycache__" in path.parts:
|
||||||
|
continue
|
||||||
|
try:
|
||||||
|
tree = ast.parse(path.read_text(encoding="utf-8"))
|
||||||
|
except (SyntaxError, UnicodeDecodeError):
|
||||||
|
continue
|
||||||
|
|
||||||
|
classes = {
|
||||||
|
alias.asname or alias.name: alias.name
|
||||||
|
for node in ast.walk(tree)
|
||||||
|
if isinstance(node, ast.ImportFrom)
|
||||||
|
and node.module
|
||||||
|
and (node.module == "OCP" or node.module.startswith("OCP."))
|
||||||
|
for alias in node.names
|
||||||
|
}
|
||||||
|
if not classes:
|
||||||
|
continue
|
||||||
|
|
||||||
|
# local variable -> OCP class, from `v = Klass(...)`
|
||||||
|
env: dict[str, str] = {}
|
||||||
|
for node in ast.walk(tree):
|
||||||
|
if isinstance(node, ast.Assign) and isinstance(node.value, ast.Call):
|
||||||
|
fn = node.value.func
|
||||||
|
cls = classes.get(getattr(fn, "id", ""))
|
||||||
|
if cls:
|
||||||
|
for tgt in node.targets:
|
||||||
|
if isinstance(tgt, ast.Name):
|
||||||
|
env[tgt.id] = cls
|
||||||
|
|
||||||
|
def owner(node: ast.expr) -> str | None:
|
||||||
|
"""The OCP class an expression evaluates to, when knowable."""
|
||||||
|
if isinstance(node, ast.Name):
|
||||||
|
return env.get(node.id)
|
||||||
|
if isinstance(node, ast.Call):
|
||||||
|
return classes.get(getattr(node.func, "id", ""))
|
||||||
|
return None
|
||||||
|
|
||||||
|
for node in ast.walk(tree):
|
||||||
|
if not isinstance(node, ast.Attribute):
|
||||||
|
continue
|
||||||
|
cls = owner(node.value)
|
||||||
|
if cls:
|
||||||
|
direct[cls].add(node.attr)
|
||||||
|
continue
|
||||||
|
# <known>.Outer().Inner — constrains Outer's return type
|
||||||
|
inner = node.value
|
||||||
|
if isinstance(inner, ast.Call) and isinstance(inner.func, ast.Attribute):
|
||||||
|
cls = owner(inner.func.value)
|
||||||
|
if cls:
|
||||||
|
chained[f"{cls}.{inner.func.attr}()"].add(node.attr)
|
||||||
|
|
||||||
|
def dump(title: str, data: dict[str, set[str]]) -> None:
|
||||||
|
keys = [k for k in sorted(data) if only is None or only in k]
|
||||||
|
if not keys:
|
||||||
|
return
|
||||||
|
print(f"\n{title}")
|
||||||
|
for key in keys:
|
||||||
|
print(f" {key}: {', '.join(sorted(data[key]))}")
|
||||||
|
|
||||||
|
dump("instance methods, by class:", direct)
|
||||||
|
dump("chained calls (constrain the return type):", chained)
|
||||||
|
return 0
|
||||||
|
|
||||||
|
|
||||||
def emit(app_root: pathlib.Path, out: pathlib.Path) -> int:
|
def emit(app_root: pathlib.Path, out: pathlib.Path) -> int:
|
||||||
data = scan(app_root)
|
data = scan(app_root)
|
||||||
out.write_text(json.dumps(data, indent=2) + "\n")
|
out.write_text(json.dumps(data, indent=2) + "\n")
|
||||||
@@ -130,10 +215,18 @@ def main() -> int:
|
|||||||
ap.add_argument("--emit", action="store_true", help="rewrite the inventory")
|
ap.add_argument("--emit", action="store_true", help="rewrite the inventory")
|
||||||
ap.add_argument("--check", action="store_true",
|
ap.add_argument("--check", action="store_true",
|
||||||
help="compare the installed OCP against the inventory")
|
help="compare the installed OCP against the inventory")
|
||||||
|
ap.add_argument("--methods", action="store_true",
|
||||||
|
help="report the instance methods the app calls per class")
|
||||||
|
ap.add_argument("--only", help="with --methods: substring filter on the class")
|
||||||
ap.add_argument("--app", type=pathlib.Path, default=DEFAULT_APP)
|
ap.add_argument("--app", type=pathlib.Path, default=DEFAULT_APP)
|
||||||
ap.add_argument("--inventory", type=pathlib.Path, default=DEFAULT_INVENTORY)
|
ap.add_argument("--inventory", type=pathlib.Path, default=DEFAULT_INVENTORY)
|
||||||
args = ap.parse_args()
|
args = ap.parse_args()
|
||||||
|
|
||||||
|
if args.methods:
|
||||||
|
if not args.app.is_dir():
|
||||||
|
print(f"app sources not found: {args.app}", file=sys.stderr)
|
||||||
|
return 2
|
||||||
|
return methods(args.app, args.only)
|
||||||
if args.emit:
|
if args.emit:
|
||||||
if not args.app.is_dir():
|
if not args.app.is_dir():
|
||||||
print(f"app sources not found: {args.app}", file=sys.stderr)
|
print(f"app sources not found: {args.app}", file=sys.stderr)
|
||||||
|
|||||||
@@ -51,3 +51,10 @@ print('modules:', len(OCP._OCP.__all_modules__))
|
|||||||
echo "--- coverage against the app's symbol inventory ---"
|
echo "--- coverage against the app's symbol inventory ---"
|
||||||
uv run python "$HERE/inventory.py" --check || \
|
uv run python "$HERE/inventory.py" --check || \
|
||||||
echo "(incomplete coverage is expected until the increments land)"
|
echo "(incomplete coverage is expected until the increments land)"
|
||||||
|
|
||||||
|
# The binding's own suite, run from the swapped venv: this is what exercises the
|
||||||
|
# published artifact inside the app's exact dependency set. It imports only
|
||||||
|
# OCP/n3xd_ocp, so it works long before the app's own tests can collect (their
|
||||||
|
# conftest imports n3xd.main, i.e. the whole OCP surface).
|
||||||
|
echo "--- ocp suite under the swapped venv ---"
|
||||||
|
uv run pytest "$OCP_REPO/tests" -q
|
||||||
|
|||||||
Reference in New Issue
Block a user