[FIX]: Enforce PromptTemplate construction invariants - #132
Spencer Schoenberg (spencrr) wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| instance = object.__new__(cls) | ||
| object.__setattr__(instance, "name", definition.name) # ruff:ignore[unnecessary-dunder-call] | ||
| object.__setattr__( # ruff:ignore[unnecessary-dunder-call] | ||
| instance, | ||
| "description", | ||
| definition.description, | ||
| ) | ||
| object.__setattr__( # ruff:ignore[unnecessary-dunder-call] | ||
| instance, | ||
| "parameter_keys", | ||
| tuple(definition.parameters), | ||
| ) | ||
| object.__setattr__(instance, "_template", compiled) # ruff:ignore[unnecessary-dunder-call] | ||
| return instance |
There was a problem hiding this comment.
Was considering your comment on whether dataclass is still necessary since we are limiting some of its core usage...what do you think about this approach?
The object.__new__ + four object.__setattr__ reconstruction here only exists to work around init=False on a frozen, slotted dataclass — the class opts into the dataclass machinery and then fights it. You can keep the dataclass (its generated __init__, __repr__, frozen guard, and slots) and just gate construction with a module-private sentinel:
_FROM_YAML: Final = object()
@final
@dataclass(frozen=True, kw_only=True, slots=True, eq=False)
class PromptTemplate:
name: str
description: str | None
parameter_keys: tuple[str, ...]
_template: Template = field(repr=False)
_token: InitVar[object]
def __post_init__(self, _token: object) -> None:
if _token is not _FROM_YAML:
raise TypeError("Use PromptTemplate.from_yaml()")from_yaml then just calls cls(name=..., description=..., parameter_keys=tuple(definition.parameters), _template=_compile_template(definition, path=path), _token=_FROM_YAML). That deletes _from_validated_definition, both object.* calls, the custom raising __init__, and all four lint suppressions — for the same guarantees.
I checked this on 3.11: direct construction, bare PromptTemplate(), and a wrong/absent token all raise TypeError; instances stay frozen (FrozenInstanceError); equality stays identity-based; the repr still hides _template; and because _token is an InitVar it never lands in __slots__ or on the instance. If you'd rather not carry a sentinel field, the other option is dropping @dataclass for a plain @final + __slots__ class — but then you hand-roll frozen and __repr__, so this is the smaller change.
There was a problem hiding this comment.
Nina Chikanov (@nina-msft) Yeah I am not a big fan of the token/sentinel thing. Can you PTAL at #144 - maybe a cleaner approach?
There was a problem hiding this comment.
If we go with #144 - can you close this out as stale?
There was a problem hiding this comment.
Closing both in favor of your comment! Stay tuned!
|
See #156 |
## Description This supersedes the constructor approaches in #132 and #144 and follows the review feedback on #144. This PR makes prompt-template construction source-neutral while retaining `PromptTemplate.from_yaml(path)` as the YAML and filesystem adapter. - Adds a public, strict, frozen `PromptTemplateDefinition`. - Normalizes declared parameters to an immutable, ordered tuple. - Prevents prompt source from appearing in definition or template representations. - Replaces injectable dataclass construction with `PromptTemplate(definition=...)`. - Compiles and validates Jinja exclusively inside `PromptTemplate`. - Preserves `from_yaml(path)` and both existing production call sites. - Keeps YAML paths outside the definition and compiled runtime object. - Preserves direct underlying exception causes while adding YAML path context. - Reports Jinja line numbers explicitly as template-value lines rather than misleading physical YAML lines. - Replaces metadata-based equality with object identity. Jinja receives neither the YAML path nor the logical template name. This avoids incorrect YAML file/line attribution and prevents names such as `summary.html` from unexpectedly enabling autoescape. ## Breaking changes Direct component-wise construction is no longer accepted: ```python PromptTemplate( name="Greeting", description=None, parameter_keys=("subject",), _template=compiled_template, ) ``` Construct from a validated definition instead: ```python PromptTemplate( definition=PromptTemplateDefinition( name="Greeting", description=None, parameters=("subject",), value="Hello, {{ subject }}!", ), ) ``` `PromptTemplate.from_yaml(path)` is unchanged, so existing supported call sites require no migration. Separate `PromptTemplate` instances now use identity equality and hashing rather than metadata-based structural equality. `PromptTemplateDefinitionError.path` is optional for in-memory construction. ## Checklist - [x] `pre-commit run --all-files` passes - [x] Tests added or updated for changes - 34 focused prompt-template tests pass - 652 tracked unit tests pass - [x] Documentation updated - Module and public API docstrings describe the source-neutral construction and error contracts
Description
Fixes construction and equality invariants for
PromptTemplateintroduced in #120.The dataclass-generated initializer exposed every field, including
_template. A leading underscore is only a Python naming convention, so a caller could combine declared parameter metadata with an unrelated Jinja template and bypassPromptTemplate.from_yaml()validation:This renders an empty string: argument validation succeeds against
parameter_keys, while the injected template references a different variable and was compiled outside RAMPART'sStrictUndefinedenvironment.This PR:
PromptTemplate()call left available byinit=Falsealone;object.__new__()andobject.__setattr__()for the narrow initialization phase required by a frozen, slotted dataclass;_templatewas excluded from comparison, while preserving identity-based hashing; andPromptTemplateas final for static type checkers and IDEs.PromptTemplate.from_yaml(path)remains the supported construction API and retains itsSelfreturn type. Separate instances now use identity equality:Breaking changes
Direct
PromptTemplate(...)construction is now rejected. UsePromptTemplate.from_yaml(path)instead. Equality between separate instances is now identity-based rather than metadata-based. Existing RAMPART production call sites already construct templates throughfrom_yaml().Checklist
pre-commit run --all-filespasses