Replace Runtime Monkeypatching with Generated WIT Binding Wrappers#84
Replace Runtime Monkeypatching with Generated WIT Binding Wrappers#84posborne wants to merge 18 commits into
Conversation
This is a pretty direct wrapping of the WIT with the addition of The composite `EdgeRateLimiter` which matches what is provided by the Rust SDK with `ERL`. Viceroy mostly has stubs for this functionality at this point in time, so the tests are not particularly substantive but do try to at least put some tracers through the hostcall boundary. There are several xfail tests which could be made to fail properly with viceory changes to do more faithfully match parts of the production impl.
Lints for python 3.14 disallow string quoting types; importing annotations from __future__ defers type avaluations to avoid this problem and make the linter happy.
For now, I simplified the exception documentation to reference base exceptions as some of the detailed sub-exceptions were not accurate or complete. I think documenting more generally when subclasses of the base exception may occur generally will likely be more sustainable unless an exception is on a path that is likely to need to be frequently handled by guest code.
Introduce scripts/generate_bindings/ which produces fastly_compute/_bindings/*.py for all Fastly Compute WIT interfaces. Each generated module wraps its wit_world counterpart with remap_wit_errors applied at definition time, corrected :raises: docstrings referencing the SDK exception hierarchy, and proper Python type annotations including resource handle and tuple wrapping. Delete runtime_patching/ and migrate all SDK code (config_store, erl, log, wsgi, requests) off direct wit_world imports onto the generated bindings layer.
WIT list<u8> is always a byte buffer; bytes is the correct Python type for callers to pass and type checkers to verify. This fixes annotations throughout http_body, http_req, http_resp, cache, secret_store, log, and related interfaces, and removes the now-unnecessary bad-override suppress on LogEndpoint.write.
_all_type_refs did not recurse into Record fields, so enum, variant, and resource types that appeared only as record field types were invisible to _reexports_for_interface and _extra_imports_for_interface. Fix by walking record fields after adding a Record to the result set. Affected: InsertMode/ListMode (kv_store), ReplaceStrategy/Request (cache), Backend (http_cache), IpAddress (security). Removes the F821 suppression from pyproject.toml per-file-ignores for _bindings/.
Generate __all__ in each _bindings module to define the public contract:
own-interface records, resources, freestanding functions, and
enum/flags/variant re-exports, excluding Extra* extensibility types and
infrastructure names (MAPPINGS, remap_wit_errors, FastlyResource).
Foreign resources imported from other _bindings modules (e.g. Pollable)
are also excluded so each type has a single canonical import location.
Add thin public fastly_compute/{module}.py files that re-export via
'import *', letting __all__ do the filtering. Skipped: http_body (raw
handle plumbing), http_incoming (framework entry point), http_types and
types (empty placeholders). Existing hand-written modules (config_store,
erl, log, wsgi, requests) are unchanged.
…ed bindings Generate :raises entries in method docstrings derived from the WIT result error type. Each decorated method now documents the parent exception class for its error type (e.g. KvError, OpenError, Error) rather than listing every individual case, keeping docstrings concise while pointing readers to the right catchable type. UnexpectedFastlyError is omitted per-method since it applies universally and is documented on remap_wit_errors itself. Methods with no WIT docs get a minimal name-based summary so :raises lines are never orphaned against the opening quotes. Also add DocsHaver.docs_for_python() which rewrites high-confidence WIT idioms to Python equivalents before embedding in docstrings: ok(some(X)) -> X, ok(none)/none -> None, true/false -> True/False, and kebab-case identifiers in backticks -> snake_case. docstring_with_raises() uses this instead of docs() so all generated method docstrings get the rewrites. Fix Makefile to include scripts/generate_patches/*.py as a dependency of the _bindings/__init__.py target so changes to wit.py correctly trigger regeneration.
This cleans up some of the odd bits we get as a result of trying to format the code properly in all cases with Jinja -- it's easier to just fix many of these details up in post.
Change WsgiHttpIncoming.handle() to declare raw WIT types for its parameters (_wit_http_req.Request, _wit_async_io.Pollable) rather than _bindings wrapper types, since componentize-py passes raw resources at the export boundary. This accurately reflects what actually arrives and eliminates the bad-override suppression. Use distinct variable names (wrapped_request, wrapped_body) for the _bindings-wrapped values so the type checker can track the type transition without casts. Removes all three inline pyrefly: ignore comments from the file.
RecordField now extends DocsHaver, giving it access to docs(), docs_for_python(), and docstring(). A new param_doc() method normalises the field docs to a single line (collapsing whitespace) for use in :param entries. The bindings template emits an __init__ docstring with :param lines for every non-extra field that carries WIT docs, and omits the docstring entirely when no fields have docs. WIT-to-Python prose rewrites from docs_for_python() are applied (none/true/false capitalisation, ok()/some() unwrapping, kebab-case to snake_case).
We now no longer do monkeypatching and some of the existing code was no longer called or named in a misleading fashion.
For internal code paths like the requests facade, dogfood our own top-level api surface (which may just be a re-export) for building this second layer of functionality.
The monolithic wit/__init__.py had become quite large; take a stab at separating with a few changes to break circular dependencies between wit/types around interface names.
|
One missing piece here that I think we can likely generate in a reasonable fashion is for enumerations like
|
|
Just want to let you know this is on my radar, but it will have to wait for a nice chunk of uninterrupted thought. |
erikrose
left a comment
erikrose
left a comment
There was a problem hiding this comment.
Here are a few things to start with. This is a huge dump to trawl through. Next time, can we meet beforehand and hash out philosophy rather than attempting design-by-6000-line-surprise?
| ) -> wit_backend.Backend: | ||
| options = wit_backend.DynamicBackendOptions() | ||
| ) -> Backend: | ||
| options = DynamicBackendOptions.new() |
There was a problem hiding this comment.
If I were a Python SDK customer, I'd rather have idiomatic Python than consistency with other languages I'm not using. I imagine mechanical translation is straightforward given our consistency of naming.
| Combines a :class:`RateCounter` and :class:`PenaltyBox` into a single | ||
| interface for simplified rate limiting operations. | ||
|
|
||
| :param rate_counter: Rate counter to use for counting |
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, yes, I had started to play with some of these ideas based on the ERL branch but didn't fully address a few comments.
| @@ -63,10 +63,7 @@ def open(cls, name: str) -> Self: | |||
| """Open a rate counter by name. | |||
There was a problem hiding this comment.
I think documenting more generally when subclasses of
the base exception may occur generally will likely be more sustainable
unless an exception is on a path that is likely to need to be frequently
handled by guest code.
Sorry, couldn't make sense of this. Can you elucidate?
My initial position is that open-error's cases have intuitive-enough applicability to ERL's things that we can generally say "raises OpenError" and be done with it. Maybe that's what you meant.
I'd think twice before doing that with plain old error, as its has cases that aren't as clear.
There was a problem hiding this comment.
Yeah, that reads odd. What I was trying to express is, I think, the same basically. Pointing readers at OpenError is probably good enough and we could consider adding whatever text makes sense on OpenError or its subclasses to understand how to handle things generally rather than trying to call out each individual error inline.
For cases that might reasonably need to be able to be handled on a given API call, having the WIT docs describe that case (e.g. If the name is greater than 200 characters, then we'll return this-error or whatever) would be ideal and we can do our pass that info along, possibly with a mapping of error->exception (though even if we don't, I think most readers will probably be able to follow).
| :return: True if the entry is rate limited, False otherwise | ||
| :raises ~fastly_compute.exceptions.types.error.InvalidArgument: If parameters are invalid | ||
| :raises ~fastly_compute.exceptions.types.error.GenericError: If an unexpected error occurs | ||
| :raises ~fastly_compute.exceptions.FastlyError |
There was a problem hiding this comment.
We will have to be somewhat general here because the WIT comments don't nail down which cases can be raised from each routine, but we can at least claim to raise Error (class Error(FastlyError)), from which the full range of cases can be discovered.
Happy to discuss; this work was in large part my attempt to see what I could achieve in a day to flesh out several ideas. Given that somewhat limited time investment, I'm not completely tied to things here, but this was the easiest way for me to explore several possible features in a way that can be discussed. The ideas related to codegen vs. monkeypatching had been discussed several times previously. Much of the code here is generated mechanically. |
2 participants
Disclaimer: this is a large change, but a lot of the noise is from restructuring bits from monkeypatching contributing to the line noise. There is still a lot of new code; I think this is a very compelling path forward but am open to trying to make the change smaller -- that being said, reviewing the generated
_bindingslayer should provide a solid surface to look at and work backwards from.Background
Previously the SDK addressed this with runtime monkeypatching — at startup, a generated
patch()function wrapped eachwit_worldfunction with a decorator that caughtErrvalues and re-raised them as typed Python exceptions. This approach worked, and its simplicity is worth acknowledging: the WIT bindings were used directly, a small runtime layer handled error translation, and there was little generated code to reason about.This PR replaces that approach with generated binding wrappers: a code generation pipeline that produces a
fastly_compute/_bindings/layer at SDK build time, wrapping each WIT resource and function with typed, documented Python classes.In addition to strictly making the transition to codegen, I also tried to address some of the main missing pieces in order to be able to directly re-export the generated wrappers by doing some docs modifications (adding
:raisesand rewriting witicisms), doing type mapping between wrapper types and wit types (_wit_resource) where required, and adding in functionality that was missing aroundRecordtypes using the builder pattern so we have something to work against there which includes docs and (hopefully) reasonable defaults.Why move away from monkeypatching?
No IDE support. Users worked with raw
wit_worldtypes. Autocomplete and hover docs showedWIT internals, not SDK types.
:raisesdocumentation was impossible to generate.Documentation required a separate effort. The monkeypatching approach gave Sphinx nothing
useful to introspect — generating API docs would have required a separate codegen pass or
external stub files regardless. This PR instead treats the WIT definitions as the source of
truth and surfaces as much as possible directly into the Python layer: docstrings,
:raisesentries,
:paramdocs on record fields, and WIT prose rewritten for Python idioms. As theWIT definitions gain richer documentation, the generated Python layer improves automatically
with no manual wrapping needed.
Runtime fragility.
patch()had to be called before any SDK call; the ordering dependencywas implicit and not enforced by the type system.
Static analysis blind spots. Type checkers had no visibility into the exception behaviour
of patched functions, since patching happened at runtime via attribute mutation.
Scaling maintenance. Every new WIT interface required manually updating the patch list.
Ease of Reasoning. While tracing through a fastly_compute API call still involves an
intermediate layer, I find the more direct delegation to be easy to understand without
having to untangle (for both humans and machines) what is going on at import and runtime.
Delegation vs. inheritance
The natural question when wrapping a type is whether the wrapper should subclass the WIT
type or hold a reference to it (delegation via
FastlyResource[T]).Subclassing was considered — componentize-py types are subclassable in Python — but the wrapper
and the WIT type are not in a true
is-arelationship:Method signatures diverge.
Store.insert()in WIT accepts a rawwit_world.imports.async_io.Pollable; our wrapper accepts_bindings.Pollable. These areunrelated types (
_bindings.Pollabledoes not inherit fromwit_world.imports.async_io.Pollable),so overriding to accept the wrapper type violates the Liskov Substitution Principle and produces
bad-overrideerrors from every type checker. The same applies to return types —Store.lookup()in WIT returnswit_world.imports.kv_store.Entry; our wrapper returns_bindings.Entry.WIT type constructors are opaque. componentize-py's
__init__signatures are(*args, **kwargs);they are not designed to be constructed by user code, making
super().__init__()calls unsafe.Delegation keeps the WIT type as a private implementation detail. The cost is one additional
layer in stack traces and unwrapping boilerplate at call sites (
param._wit_resource), which thegenerator handles automatically.
What the generator produces
For each WIT interface,
scripts/generate_bindings/produces afastly_compute/_bindings/*.pymodule containing:
Resource wrapper classes —
FastlyResource[T]subclasses with methods delegating to theunderlying WIT resource. Methods whose WIT signature returns
result<_, E>are decorated with@remap_wit_errors(MAPPINGS)at class definition time, not patched at runtime.Record wrapper classes — typed
__init__signatures with default values and:paramdocumentation drawn directly from WIT field docs (e.g.
WriteOptions,InsertOptions).Enum/variant/flags re-exports — a single import surface, no
wit_worldimports needed.__all__declarations — explicit public names, excludingExtra*extensibility types(WIT ABI forward-compatibility handles, always
Nonein practice). As needed, we cantailor this in codegen or at the final shim (which largly re-exports with
*now).Docstrings with
:raisesentries — each method that can raise gets a:raises ~fully.qualified.ParentExceptionClass:entry, pointing to the catchable parentrather than listing every variant case individually. We will probably want to bolster
the error case documentation in WIT and in our exception hierarchy to aid users further.
WIT-to-Python prose rewrites — WIT idioms rewritten before embedding in docstrings:
`ok(some(v))`→`v`,`none`→`None`,`true`/`false`→`True`/`False`, kebab-case identifiers → snake_case. None of this was possibleunder monkeypatching, where docstrings were never touched. This is limited to a few
regex patterns today but can be expanded as we see fit.
Public API surface
Thin public modules (
fastly_compute/kv_store.py,fastly_compute/backend.py, etc.) re-exportfrom
_bindingsviaimport *, with__all__doing the filtering:fastly_compute.kv_storeetc., never from_bindingsdirectly.__init__.pyremains empty — modules are only loaded when explicitly imported, avoidingunnecessary memory in the compiled Wasm binary snapshot (componentize-py snapshots all
top-level imports).
config_store,erl,log,requests,wsgi)are unchanged at the user-facing level.
http_bodyandhttp_incomingare intentionally excluded for now — raw handle plumbingand the framework entry point, not intended for direct use.
Code generation restructuring
scripts/generate_patches/has been renamed and split intoscripts/wit/(shared WITabstraction layer, used by both generators) and
scripts/generate_exceptions/(exceptionhierarchy generation, formerly the only active part of
generate_patches). The deadgenerate_patches()function andpatches.py.jinjatemplate — which generated the now-deletedruntime_patching/patches.py— have been removed along with all their supporting code.Tradeoffs
Improvements:
:raisesentries everywheremakeinvocationOngoing costs:
_bindingsare one layer deeper than before, though we were headed toward addingsome kind of wrapper layer manually in many cases.
# automatically generatedheader andmakeregeneration)FastlyResource[T]delegation pattern adds indirection that subclassing would avoid —but as described above, clean subclassing is not achievable given how componentize-py generates
its types