feat: produce and transport resolved symbol tables from Python - #332
Conversation
Signed-off-by: David Leong <leongdl@amazon.com>
Signed-off-by: David Leong <leongdl@amazon.com>
| /// | ||
| /// Use this to move a table across a process or service boundary; | ||
| /// pair it with ``from_json_str`` to reconstruct. The result is | ||
| /// stable for a given table, so it is safe to store or compare. |
There was a problem hiding this comment.
The two guarantees documented here — an array of name/type/value objects "in canonical (lexicographic) path order", and "stable for a given table, so it is safe to store or compare" — only hold for instances built via from_symtab.
from_json_str is explicitly lazy (per its own docstring, and confirmed by test_from_json_str_defers_content_validation_to_to_symtab, which stores a non-array document successfully). So inner can hold arbitrary well-formed JSON, and to_json_str re-emits it as-is. Consequences:
to_json_str()on afrom_json_str-derived instance is not necessarily an array, and not necessarily in canonical path order.- Byte-comparing two
to_json_str()outputs is not equivalent to comparing table contents: a hand-built transport string with entries in non-lexicographic order, or with duplicate names, survives the round trip and compares unequal to thefrom_symtabform of the same table.
Since the docstring actively invites callers to store and compare the text, consider scoping the claim (e.g. canonical order and byte-stability hold for tables built via from_symtab; from_json_str preserves the input text as given), or normalizing on ingest in from_json_str so the invariant is unconditional.
| ) | ||
|
|
||
| # THEN | ||
| assert "RawParam.Scene" in restored |
There was a problem hiding this comment.
This test does not exercise PATH-typed values, so it cannot catch a regression in path serialization.
SymbolTable({"RawParam.Scene": "/proj/scene.blend"}) goes through py_to_expr_value with no target type, which infers the native type — i.e. a plain STRING, not PATH. path_format is then irrelevant to the round trip. The existing test at line 43 shows the way to actually build a PATH value:
ExprValue(str(input_file), type="path", path_format=HOST_PATH_FORMAT)Separately, the only assertion is membership ("RawParam.Scene" in restored), which holds for any surviving key. Asserting the restored value and its type code (e.g. restored["RawParam.Scene"].type == TypeCode.PATH plus the expected host-format string) would make the test meaningful — and would be the one place in this suite verifying that type: "path" survives the JSON transport with the requested path_format applied on the way back.
| second = SerializedSymbolTable.from_symtab(symtab).to_json_str() | ||
|
|
||
| # THEN | ||
| assert first == second |
There was a problem hiding this comment.
This asserts determinism (same input, same output twice in one process) but not the canonical-ordering property the docstring actually promises. Both from_symtab calls receive the identical dict, so the test would still pass if the transport order tracked insertion order rather than being sorted.
To pin down "canonical (lexicographic) path order" — and catch nondeterminism from the underlying HashMap iteration order, which is the real risk here — compare two tables built with opposite insertion orders:
first = SerializedSymbolTable.from_symtab(SymbolTable({"Param.B": 2, "Param.A": 1})).to_json_str()
second = SerializedSymbolTable.from_symtab(SymbolTable({"Param.A": 1, "Param.B": 2})).to_json_str()
assert first == second(Python dict literals preserve insertion order, so these two do reach the Rust side in different orders.)
| # THEN | ||
| assert via_json.symbols == via_pickle.symbols | ||
| for name in via_json.symbols: | ||
| assert via_json[name] == via_pickle[name] |
There was a problem hiding this comment.
This test is circular and cannot fail. __reduce__ (symbol_table.rs:342) serializes with the same serde_json::to_string(&self.inner) that to_json_str uses, and _reconstruct_serialized_symtab calls the same SerializedSymbolTable::from_json_str. So both sides of the comparison are literally the same code path — as the docstring itself notes ("round-trips through the same transport text"). It adds test surface without adding coverage.
If the intent is to guard against pickle and JSON diverging in the future, comparing the two text forms directly would be a tighter check:
assert pickle.loads(pickle.dumps(serialized)).to_json_str() == serialized.to_json_str()Otherwise dropping it leaves nothing untested — test_round_trip_preserves_values already covers JSON content fidelity.
| )) | ||
| })?; | ||
| Ok(Self { inner }) | ||
| } |
There was a problem hiding this comment.
Design note on the lazy-validation choice: for the use case the docstring names — a scheduler persisting the transport text and later handing it to a worker — deferring content validation to to_symtab pushes the error to the worst possible place. The scheduler happily accepts and stores a bad payload, and the failure surfaces later on the worker, far from the input that caused it.
Validating structure at ingest (i.e. parsing into the real entry type rather than a loose JSON document) would make from_json_str a real trust boundary and give the error at the point where the caller still has the offending text in hand. If the laziness is deliberate — e.g. openjd_expr::SerializedSymbolTable::from_json_str intentionally keeps the raw document so unknown future entry kinds pass through untouched — it would help to say so in the docstring, since as written it reads as an accident rather than forward-compatibility.
Signed-off-by: David Leong <leongdl@amazon.com>
| @staticmethod | ||
| def _entries(serialized: Any) -> dict[str, tuple[str, Any]]: | ||
| """Decode the transport form into {name: (type, value)}.""" | ||
| _helper, (text,) = serialized.__reduce__() |
seant-aws
left a comment
There was a problem hiding this comment.
PR #332 Review: create_job_with_symbol_tables + JSON accessors
Verdict: Approve ✅ — good to merge.
Executive Summary
Clean, well-designed PR that adds two features we directly consume:
SerializedSymbolTable.from_json_str()/.to_json_str()— the public JSON transport API (replaces the private_reconstruct_serialized_symtabwe're currently using in #1063)create_job_with_symbol_tables()— DataPlane-side producer returning resolved tables per-step
Architecture is sound, 17 tests cover the critical paths, and the PR was verified against openjd-rs with zero semantic mismatches across 21 step comparisons.
No Critical Issues
The reviewer bot flagged the to_json_str Rust method as potentially missing a return — this is a false positive. The actual code uses a tail expression (serde_json::to_string(&self.inner).map_err(...)) with no semicolons, which is valid Rust implicit return of PyResult<String>.
Important Issues (should discuss, not blocking)
| # | Issue | Severity | Assessment |
|---|---|---|---|
| 1 | Deferred validation in from_json_str — validates JSON syntax only, pushes content errors to to_symtab() call time |
Important | This is fine for our consumption path because we call to_symtab() immediately after deserializing. But worth a docstring clarifying that callers should validate early. |
| 2 | Bot comment #2 — stability claim scope | Minor | The "canonical order" guarantee holds for from_symtab-built instances but not from_json_str-derived ones. A docstring scoping the claim would help. |
|
Also have this as a followup: aws-deadline/deadline-cloud-worker-agent#1063 |
Two commits, both in service of one use case: letting a scheduler produce a
resolved symbol table and hand it to a worker on another host — the flow
SerializedSymbolTable's own docstring describes as "a scheduler serializes thesymbol table to JSON and sends it to a worker that may be on a different OS".
Today neither half of that is reachable from Python: there is no supported way to
get the transport text out, and
create_jobdiscards the tables it builds.feat: add SerializedSymbolTable JSON accessorsto_json_str()/from_json_str()feat: return resolved symbol tables from create_jobcreate_job_with_symbol_tables()→JobWithSymbolTablesHappy to split these into two PRs if you'd prefer to review them separately.
1. JSON accessors
SerializedSymbolTableexposesfrom_symtab,to_symtaband__reduce__. Tomove the transport text across a boundary today, a caller has to reach into the
pickle protocol:
or call the private module-level pickle helper to get back in:
The Rust side already has the capability (
from_json_str, andserde_jsonvia thetype's
Serializeimpl). This exposes it.Design notes
from_json_strvalidates syntax, not contents. Malformed JSON raisesValueError; a well-formed document that is not a valid table is accepted andrejected later by
to_symtab. That matches the existing split —to_symtabiswhere entry validation lives — and keeps
from_json_strcheap. Tests pin bothhalves so the behaviour is deliberate.
__reduce__is unchanged, still routing through_reconstruct_serialized_symtab, so existing pickles keep working. Happy tore-point it and drop the private helper if you prefer, but that changes what old
pickled bytes reference.
SymbolTable'sSerializeimpl emits entries in canonicalpath order, which the type already relies on for
Eq/Hash, so the text is safeto store and compare.
2. Returning the resolved symbol tables
create_jobbuilds a job-scope symbol table, andinstantiate_modelextends itper step with
Step.Nameand the step's template-scopeletbindings(
StepTemplate._extend_step_symtab). Both are then discarded. openjd-rs keeps theequivalent table on each
Stepasresolved_symtab; the Python model has noequivalent, so a Python scheduler cannot produce what a Rust session consumes.
Design notes
Stepwould change the model's serialized form, and
Step.dict()output is persistedas a worker contract by at least one consumer. A separate return value avoids
that entirely.
create_jobis untouched behaviourally. Its body moved to_create_job_and_symbol_table, which both entry points call. A test asserts thetwo produce equal jobs.
reimplementing the step scope, this re-invokes
step_template._job_creation_metadata.extends_symtab— the same callableinstantiate_modeluses. It depends only on the step's name, itsletbindingsand the job-scope table, so re-invoking it is deterministic and cannot drift from
what the job was instantiated with.
symtab_to_expr_valuesfor the typedcoercion, then
SerializedSymbolTable.from_symtab— the same serializeropenjd-rs uses.
openjd.expris imported inside the helper and underTYPE_CHECKING, so importingopenjd.modelstill does not load the Rustbindings. Your
test_importing_parser_does_not_load_rust_expr_surfacecaught myfirst attempt at this — thanks, it's a good test.
references. This returns the full scope, which is a valid superset: a session
layers its own scopes on top either way. Filtering would be a payload
optimization and I did not want to reimplement
filter_symtab_for_stephere.Parity with openjd-rs
The point of this is that a v0 producer can feed a Rust consumer, so I measured
that rather than assuming it. 18 templates, 21 step comparisons, diffed entry by
entry on name, type and value against
openjd-rs'Step.resolved_symtab:Zero missing symbols, zero semantic mismatches. Cases covered: scalar types,
BOOL,LIST[INT],LIST[STRING],LIST[PATH],RANGE_EXPR, PATH(
RawParam-only convention), template-scopelet(frozen), script-scopelet(inputs only, results absent),
parameterSpaceranges from alet,hostRequirementsfrom alet,CHUNK[INT], step environments, multi-stepscoping, explicit parameter values, negative/zero/empty values, and the EXPR
function library.
RANGE_EXPRwas the case I expected to break, sincesymtab_to_expr_valuesomits it from
typesso the engine infers it from the value. Both sides produce('range_expr', '1-9:2'), including the same1-10:2→1-9:2normalization.test_matches_the_rust_implementationencodes this as a regression test.One cosmetic difference worth flagging. For an integral-valued float written
1.0or1.00, openjd-rs emits"1"and this Python path emits"1.0". Bothdecode to the same
ExprValue, so it is a transport-text difference rather than avalue one — confirmed by round-tripping each through
to_symtaband comparing.It still has a consequence.
SerializedSymbolTablederivesEqandHashstructurally over the JSON, so anything comparing two serialized tables byte-wise
sees a false mismatch on integral floats. Callers should compare decoded
SymbolTables instead. Normalizing float display between the two implementationswould close it at the source, but that changes transport output, which felt out of
scope here.
Testing
hatch run test— 5478 passed, 24 skipped, 3 xfailed.hatch run lintclean(ruff, black, mypy).
cargo fmt --checkandcargo checkclean.Total coverage moves from 93.91% to 93.98%; every line added is covered. The 94%
gate was already short before this change.
17 new tests: 9 for the JSON accessors in
test/openjd/expr/test_symbol_table.py, 8 for the symbol tables intest/openjd/model_v0/test_create_job.py.Two things to flag
scripts/generate_stubs.shdoes not run on currentmainlinefor me, so Ihand-wrote the two stub entries in
_openjd_rs.pyito match the generator's styleand verified with
mypy. The script clonespyo3-stub-genat--depth 1with nopinned revision, and today's tip no longer compiles against this repo's pyo3:
Pinning that clone to a known-good tag would make stub regeneration reproducible.
Happy to send it separately.
hatch run testfails ifCONDA_PREFIXis set alongsideVIRTUAL_ENV(
maturin failed: Both VIRTUAL_ENV and CONDA_PREFIX are set). Not caused by thischange, and easy to work around with
env -u CONDA_PREFIX, but it bites anyonedeveloping from a conda base environment.