Repository navigation
[BREAKING] FIX: Keep a private copy's endpoint and credentials the same as its source's (part 17) #3984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[BREAKING] FIX: Keep a private copy's endpoint and credentials the same as its source's (part 17) #3984
Changes from all commits
5bba87a
b6ad630
0063be7
acfd9eb
c8c4c26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,10 @@ class Converter(Identifiable): | |
| #: Constructor parameters whose controls must preserve line breaks. | ||
| MULTILINE_PARAMETER_NAMES: ClassVar[frozenset[str]] = frozenset() | ||
|
|
||
| #: Constructor parameters that choose where requests go or how they authenticate. A private | ||
| #: copy built from API input keeps its source's values for these. | ||
| COPY_LOCKED_PARAMETER_NAMES: ClassVar[frozenset[str]] = frozenset() | ||
|
Comment on lines
+67
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every converter can be copied automatically (constructor capture, base Could we add one test: every copyable class with a parameter from |
||
|
|
||
| #: Capability requirements placed on the converter's target (if any). | ||
| #: Subclasses that use a target should override this and pass the target to | ||
| #: ``super().__init__(converter_target=...)`` so the base class can validate it. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,10 @@ class OpenAITarget(PromptTarget): | |
| # such as Ollama or vLLM may need no key at all. | ||
| supported_auth_modes: ClassVar[tuple[AuthMode, ...]] = ("api_key", "identity", "none") | ||
|
|
||
| COPY_LOCKED_PARAMETER_NAMES: ClassVar[frozenset[str]] = frozenset( | ||
| {"endpoint", "auth_mode", "api_key", "headers", "httpx_client_kwargs"} | ||
| ) | ||
|
Comment on lines
+67
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that target copies with these overrides are only built for a dry run, does the target half belong in this PR? The only target copies that send are temperature bindings, and the open #3506 stops creating them in favor of a per-request setting (it keeps only replay of saved ones). Unless something that sends target copies with these overrides is planned, I'd leave the target lock out, along with its test changes (including the #3809 update), and add it with that consumer. |
||
|
|
||
| model_name_environment_variable: str | ||
| endpoint_environment_variable: str | ||
| api_key_environment_variable: str | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add this rule to the restrictions in the backend README's Input Validation and Trust Boundary section, and state exactly what it guarantees? A copy keeps its source's connection, but creating a new target can still send server keys to an endpoint the caller chooses.