-
Notifications
You must be signed in to change notification settings - Fork 28
feat: adds a parameter object for the action object #238
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?
Conversation
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
|
Another note: I threw copilot at implementing this proposal in Clio and it made an interesting "suggestion". Use the values field for a parameter defined as sourced from environment when the environment variable is not found. This also begs the question. Of what should happen when the environment variable is not available? Error? Fallback when available and error if not? Tool specific? |
|
I like the idea of adding a default value if the source is environment. I'd probably name it default or defaultValue and not just value. |
versions/1.1.0-dev.md
Outdated
| | <a name="action-update"></a>update | Any | If the `target` selects object nodes, the value of this field MUST be an object with the properties and values to merge with each selected object. If the `target` selects array nodes, the value of this field MUST be an entry to append to each selected array. This field has no impact if the `remove` field of this action object is `true` or if the `copy` field contains a value. | | ||
| | <a name="action-copy"></a>copy | `string` | A JSONPath expression selecting a single node to copy into the `target` nodes. If the `target` selects an object node, the value of this field MUST be an object with the properties and values to merge with the selected node. If the `target` selects an array, the value of this field MUST be an entry to append to the array. This field has no impact if the `remove` field of this action object is `true` or if the `update` field contains a value. | | ||
| | <a name="action-remove"></a>remove | `boolean` | A boolean value that indicates that each of the target objects or arrays MUST be removed from the the map or array it is contained in. The default value is `false`. | | ||
| | <a name="action-parameters"></a> | [[Parameter Object](#parameter-object)][] | An array of parameter objects to use and expand the action definition. MAY contain zero or more parameters. Parameters MUST have unique names across the collection. Default `undefined`. | |
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.
Why local to an action and not global to allow use across multiple related actions, for example when applying traits?
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.
I like @ralfhandl 's suggestion to make these global rather than within the action.
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.
The reason why I didn't make the parameters global is because having the parameters on the action makes it easy for any implementer to detect parameters are used for any given action and expansion is required.
If we want to make global parameters a thing, I think we then need to introduce a parameter reference mechanism. Where we'd mandate that the action has a reference entry for a global parameter to be used and for the expansion + interpolation to happen.
Thoughts?
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.
I like that: parameters are global, and need to be referenced by actions that use them.
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.
Are we going to make parameters global and use/reference/import them in the actions?
If the values are provided in env variables, their names are global anyway, and same name means same value(s).
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.
I'm open to it. What do we want the references to look like? we could use the $ref syntax so it feels familiar, and it opens the door to having more kinds of references, external references, etc... in the future if we need to. But the downside is that it's a bit verbose and might bring in much more than we need (again, external references, etc...)
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.
I wouldn’t use $ref and simply reference by name:
- global
parametersis an array of parameter definition objects - action-local
parametersis an array of parameter names
Similar to the tags keyword in OpenAPI.
Or use different keywords, for example usedParameters in actions.
Differentiating the field would surely make it clearer to the users. |
|
I really want to restrict parameters to a single value and avoid the complexity of matrix processing. That can be done in a wrapper script. |
|
I'll try to provide more context about the intention of this proposal. It was put together mainly for the matrix feature because it's the only use case we have for parameters. I don't think I'll push this proposal to completion without matrix in one form or another. So if you instead have suggestions to mitigate the risks you outlined without removing the feature all together, I suggest we follow that path instead. |
That would be another nice simplification:
|
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
…y-Specification into feat/interpolation
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
|
@ralfhandl thanks for the default values suggestion, I think it simplifies things a great deal. Updated to incorporate that. As for the matrix, can you please articulate the risk vector more? And maybe suggest other solutions to mitigate it than "remove matrix entirely"? There are tons of precedents in the wild for that, most broadly adopted (that I know of) being the GitHub actions workflow matrix So I'm not sure why doing the same here would constitute a riskier scenario or an anti-pattern. |
|
To me it seems extremely unlikely that the If you have three or more realistic examples where this is the case, I may reconsider. Until then I believe that most use cases for loops over parameterized actions will require groups of parameter values. Assume an API has paths for several "thingies" and we need to insert systematic descriptions for each operation on each thingy. API structure is something like paths:
/foo:
get: ...
patch: ...
/bar:
get: ...
patch: ...The descriptions we want to add are along the lines of
We would need an array of (thingy path segment, thingy name)
and an array of (method, operation purpose)
Then two nested loops over these arrays can produce the necessary matrix of actions. Which means I need arrays of "structured" parameters. |
|
I'm only half paying attention here, as this week is a crazy week for me at work. But I wanted to mention something that occurred to me while I was working on some Spectral rules recently. Spectral allows the "given" (JSON Path of the things to check) to be either a string (single JSON path), or an array of strings (JSON Paths). This can be quite handy. I wonder if that has anything to do with the discussion on "looping" here? |
|
Thank you for the additional information. Side car pattern on existing schemasOne usage of the matrix feature would be to implement a sidecar pattern at scale.
In our case (which motivated my work on this issue) we get a description from OpenAI, which contains dozens of "ToolResponse" component schemas. Azure OpenAI has an additional layer that allows you to define a "ToolResponsePolicy" using additional APIs, I'd like to avoid having to redefining the dozens of side cars. Using an overlay like this one would solve for that. overlay: 1.1.0
actions:
- description: define the side cars
target: $.components.schemas["${toolName}ToolResponsePolicy"]
update:
type: "object"
description: "The ${toolName} Tool ResponsePolicy"
properties:
tool_name:
const: ${toolName}
parameters:
- name: toolName
defaultValues:
- "MCP"
- "WebSearch"
- "SemanticSearch"
- "..."Combination with source
Another pattern related to this side car is to create additional path segments when specialized operations are required, without having to re-define everything. Which is why I used a syntax which doesn't collide with path parameters in OAI. overlay: 1.1.0
actions:
- description: Duplicate the eval_run operation for the tool definition
target: $.paths[".../models/evals/{evalId}/${toolName}/run
source: $.paths[".../models/evals/{evalId}/run
parameters:
- name: toolName
defaultValues:
- "MCP"
- "WebSearch"
- "SemanticSearch"
- "..."
# potentially another update action to add additional properties for each of the tool eval runs
# this way we don't have to re-define all the properties
# and no, we can't use a base type because most of the types are shipped by OpenAI and we're only re-using themHopefully this illustrates the need pretty well. I kept it as one dimension examples to keep it simple, but you could also have another dimension per version and a third one per environment (public, national, etc...), where each dimension would add additional properties. |
|
@baywet Thanks for the detailed example. It can also be slightly modified to support my point:
Again array of structured parameter. |
|
@ralfhandl I feel like we're having two separate conversation here. I'm arguing we should have a matrix feature, and thought you were arguing against? But now seem to want structure data support? Could you please clarify? And also provide examples? |
|
I think the matrix feature without structured parameter arrays only works for your current special cases: you are extremely lucky that targets and update objects can be parameterized with the same strings. Slightly different naming conventions would break that, and the obvious fix are arrays of value tuples or flat objects, and I have provided such examples. Adding the easily breakable variant to the spec adds complexity with little potential for use. For me it is either add the "full" solution which has similar complexity, or don't add the array feature at all and place the loops around the Overlay processor. |
|
Thank you for the additional information. If I'm following correctly, you'd essentially like to define the defaultValues as string | Record<string, string>correct? I'm open to that, what does that mean for environment variables parsing though? |
|
Correct. In an environment variable I'd pass a stringified JSON array of flat objects. |
|
I'm guessing in that case we'd do away entirely with the separator, people could simply provide a JSON array of strings for the "simple format" ? |
|
Yes, that would be a nice simplification! |
docs: makes default values records as well Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
tests: updates test values for mapping default values Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
|
@ralfhandl Thank you for the additional information. |
Co-authored-by: Ralf Handl <ralf.handl@gmail.com>
Co-authored-by: Ralf Handl <ralf.handl@gmail.com>
Co-authored-by: Ralf Handl <ralf.handl@gmail.com>
| | Field Name | Type | Description | | ||
| | ---- | :----: | ---- | | ||
| | <a name="parameter-name"></a>name | `string` | **REQUIRED** A name for the parameter that is unique per action. The name MUST match the following regular expression `[a-zA-Z-0-9]+`. | | ||
| | <a name="parameter-default-values"></a>defaultValues | [`string`] or [Map(`string`, `string`] | The values to use in case the environment variable is not defined. MAY contain zero or more entries. When values contains multiple entries, the action is effectively projected for each of the entries of the parameter. If multiple parameters are defined for any given action, parameters effectively act as a matrix definition and the action should be projected across each dimension. | |
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.
Seems like there is no way to say "don't look in the environment, just use these values". Is that intentional?
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.
That was one reservation I had as well which is why I initially had the source enum in place.
Happy to restore it. Or maybe we can have a boolean instead to ignore the environment variable.
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.
@ralfhandl I'd like your input on this one please since you helped me refactor into a simpler syntax, we kind of lost a capability in the process.
|
I am not in favour of the matrix feature. I would like us to consider simple parameters though. I'm very confused about how we're using environment variables here, or it might be a terminology problem! Environment variables: the values in the environment when the tool executes? Or something else in this context? My previous proposal for supporting environment variables was rejected because we received feedback about the difficulty of parsing dollar signs in value expressions where there might already be dollar signs in the OpenAPI content for other reasons. I notice however that we have again used dollar signs in the syntax and would like to make sure that we reach out to the wider community for their feedback before pushing through a change like this. /cc @ThomasRooney |
|
@lornajane why are you not in favour of matrix parameters? As for the syntax, happy to change it to something else if we have a better suggestion |
|
Comment from @lornajane from the call: we might be able to cut some things to make it simpler to review/implement: matrix essentially, and using the string replace in the target should be prohibited for now. |
|
@mikekistler @mikeschinkel @lornajane @ralfhandl I just rewrote the initial comment of this pull request to help clarify the vision, approach, remaining questions, etc... Please have a read, hopefully this helps us unlock further progress. |
|
|
||
| | Field Name | Type | Description | | ||
| | ---- | :----: | ---- | | ||
| | <a name="parameter-name"></a>name | `string` | **REQUIRED** A name for the parameter that is unique per action. The name MUST match the following regular expression `[a-zA-Z-0-9]+`. | |
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.
Add note that same name in different actions means same environment variable value in all actions.
Which also makes different default values questionable.
Introducing the Overlay Parameters proposal
fixes #33
fixes #136
Summary
This pull-request introduces new parameters for the action object. The values of those parameters can be read from the environment variables, or when missing from the default values field. It also introduces a string interpolation syntax that needs to avoid any conflicts (see the issues linked above) so that values can be inserted anywhere in the action.
Vision
This proposal has multiple goals:
The short-term use of this would be to enable goals 1 & 2. In the long term there might be an additional opportunity to come up with “libraries of overlays” which would distribute nice additions for OpenAPI descriptions (standard paging patterns, etc…) but I’m not trying to solve for that.
Execution
Example scenarios
Matrix
See this comment #238 (comment) or that one #33 (comment)
Environment variables
See the initial issue, which has tons of great examples. I think a key recap could be “I want to use my OpenAPI description as a template for multiple hosts deployments, and I don’t know the hosts names in advances” (think about cloud deployments with tenants, etc…). This also helps enable a set of CI/CD scenarios (like deploying a temporary API on a host to run some tests, and deploying the updated documentation for the API alongside)
1.1 Release
This should not hold the 1.1 release. I’m happy to punt it to a 1.2 release if we think it’s going to get the 1.1 release out of the door faster. And at the same time, I’d like to continue making progress on this by getting actionable feedback from the reviewers.
Implementation
It might not be clearly called out in the current proposal, but the intended implementation guideline is the following in order:
Parse the Overlay document (implementers already have most of that)
Foreach action that uses parameters
Apply all the actions, projected or regular. (implementers already have that)
This is important to enable separation of concerns, get the matrix projection right, and enable maximum reusability with the code that’s already in place for v1.0. I’m happy to rework the wording to make this clear in the specification.
Remaining questions
Do we have the right interpolation syntax?
I’m happy with any syntax really, if it meets the constraints we have. I’ve forked the discussion here #136 (comment)
Should we always read Environment variables?
Since it’s in the same “parametrization domain”, I thought it’d be nice to bundle changes together. In the initial draft of this proposal, we had the ability to have constants/avoid reading the environment variables, which we’ve lost with the latest revisions. I think we should restore that ability somehow. What about a field “ignoreEnvironmentVariable” (Boolean, default false)? When set to true, the implementer would only read the defaultValues.
Do we have the right matrix/projection syntax?
I am especially interested in the projection, matrix, and scaling aspects, as my team has been dealing with these challenges and is eager to find a solution. But it also adds great value. And users of the specification are not required to use it as it is defined.
I know this is a contentious point, and there have been suggestions to cut that aspect from the proposal. I will not put efforts behind a version of that proposal without the matrix aspect in one form or another. I’m happy to take actionable suggestions to make it better/safer/simpler/…
Do we need parameters references?
Ralf made a pertinent comment about the fact that if multiple actions had the same parameter name, they are effectively the same parameter since they’d read from the same environment variable. And he suggested hoisting them at the document level instead of the action. While this would reduce the repetitive definitions further, we’d still need to flag any given action is using parameters, so implementers can do the pre-processing. This would require defining parameter references at the action level.
I think Lorna made a pertinent comment in another context: we want to keep the complexity low to bolster adoption. I think the “global parameters” comment goes against that, with minimal gains. And this is something we can review in v2 if people are tired of re-defining the same parameters.
I hope this rewrite of the initial comment brings a lot of clarity, I spent a lot of time on it :)