Skip to content
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

Support combining multiple output variables. #737

Merged
merged 3 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .lycheeignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
https://localhost:*
http://localhost:*
http://webhook:8080
https://api.openai.com/v1/chat/completions
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Added `_type` to `termvector` and `mtermvector` ([#734](https://github.com/opensearch-project/opensearch-api-specification/pull/734))
- Added missing `cancelled` and `resource_stats` to `/_reindex/{task_id}/_rethrottle` ([#740](https://github.com/opensearch-project/opensearch-api-specification/pull/740))
- Added missing `cancellation_time_millis` to `POST /_tasks/_cancel` ([#747](https://github.com/opensearch-project/opensearch-api-specification/pull/747))
- Added support for combining output variables ([#737](https://github.com/opensearch-project/opensearch-api-specification/pull/737))

### Removed
- Removed unsupported `_common.mapping:SourceField`'s `mode` field and associated `_common.mapping:SourceFieldMode` enum ([#652](https://github.com/opensearch-project/opensearch-api-specification/pull/652))
Expand Down
4 changes: 3 additions & 1 deletion TESTING_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,9 @@ Consider the following chapters in [ml/model_groups](tests/plugins/ml/ml/model_g
```
As you can see, the `output` field in the first chapter saves the `model_group_id` from the response body. This value is then used in the subsequent chapters to query and delete the model group.

You can also supply defaults for output values, e.g. for `payload._version` used in [cluster/routing/awareness/weights.yaml](tests/routing/cluster/routing/awareness/weights.yaml).
You can combine multiple values, e.g. `task_id: ${task.node}:${task.id}`.

You can supply defaults for output values, e.g. for `payload._version` used in [cluster/routing/awareness/weights.yaml](tests/routing/cluster/routing/awareness/weights.yaml).

```
version:
Expand Down
6 changes: 3 additions & 3 deletions tests/plugins/ml/ml/connectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ chapters:
actions:
- action_type: predict
method: POST
url: 'https://${parameters.endpoint}/v1/chat/completions'
url: https://api.openai.com/v1/chat/completions
headers:
Authorization: 'Bearer ${credential.openAI_key}'
request_body: '{ "model": "${parameters.model}", "messages": ${parameters.messages} }'
Authorization: Bearer Key
request_body: '{ "model": "model", "messages": "messages" }'
response:
status: 200
output:
Expand Down
18 changes: 18 additions & 0 deletions tools/src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,21 @@ export function write_json (file_path: string, content: any, replacer?: (this: a
export async function sleep (ms: number): Promise<void> {
await new Promise<void>((resolve) => setTimeout(resolve, ms))
}

/**
* Returns a string split using a delimiter, but only up to a certain number of times,
* returning the remainder of the string as the last element if the result.
*
* @param str a string
* @param delim delimiter
* @param count max number of splits
* @returns an array of strings
*/
export function split(str: any, delim: string, count: number = 0): string[] {
if (str === undefined) return []
const parts = str.split(delim)
if (count <= 0 || parts.length <= count) return parts
const result = parts.slice(0, count - 1)
result.push(parts.slice(count - 1).join(delim))
return result
}
57 changes: 57 additions & 0 deletions tools/src/tester/OutputReference.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

import { split } from "../helpers"

export class OutputReference {
chapter_id: string
output_name: string

constructor(chapter_id: string, output_name: string) {
this.chapter_id = chapter_id
this.output_name = output_name
}

/**
* Parses a string and returns a collection of output references that it may contain.
*
* @param str a string
* @returns an array of output references
*/
static parse(str: string): OutputReference[] {
const pattern = /\$\{([^}]+)\}/g
let match
var result = []
while ((match = pattern.exec(str)) !== null) {
const spl = split(match[1], '.', 2)
result.push(new OutputReference(spl[0], spl[1]))
}
return result
}

/**
* Replaces occurrences of output references.
* Takes special care of preserving the type of the reference value if the entire string is a reference.
*
* @param str a string that may contain output references
* @param callback a callback for fetching reference values
* @returns a string with output references replaced by their values
*/
static replace(str: string, callback: (chapter_id: any, variable: any) => string): any {
let full_match = str.match(/^\$\{([^}]+)\}$/)
if (full_match) {
// the entire string is a reference, preserve the type of the returned value
const spl = split(full_match[1], '.', 2)
return callback(spl[0], spl[1])
} else return str.replace(/\$\{([^}]+)\}/g, (_, k) => {
const spl = split(k, '.', 2)
return callback(spl[0], spl[1])
})
}
}
13 changes: 4 additions & 9 deletions tools/src/tester/StoryEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import { ChapterRequest, Parameter, SupplementalChapter } from './types/story.types'
import { type StoryFile, type ChapterEvaluation, Result, type StoryEvaluation, OutputReference } from './types/eval.types'
import { type StoryFile, type ChapterEvaluation, Result, type StoryEvaluation } from './types/eval.types'
import type ChapterEvaluator from './ChapterEvaluator'
import { overall_result } from './helpers'
import { StoryOutputs } from './StoryOutputs'
Expand All @@ -17,6 +17,7 @@ import { ChapterOutput } from './ChapterOutput'
import * as semver from '../_utils/semver'
import _ from 'lodash'
import { ParsedChapter, ParsedStory } from './types/parsed_story.types'
import { OutputReference } from './OutputReference'

export default class StoryEvaluator {
private readonly _chapter_evaluator: ChapterEvaluator
Expand Down Expand Up @@ -217,10 +218,7 @@ export default class StoryEvaluator {
static #extract_params_variables(parameters: Record<string, Parameter>, variables: Set<OutputReference>): void {
Object.values(parameters ?? {}).forEach((param) => {
if (typeof param === 'string') {
const ref = OutputReference.parse(param)
if (ref) {
variables.add(ref)
}
OutputReference.parse(param).forEach((ref) => variables.add(ref))
}
})
}
Expand All @@ -229,10 +227,7 @@ export default class StoryEvaluator {
const request_type = typeof request
switch (request_type) {
case 'string': {
const ref = OutputReference.parse(request as string)
if (ref !== undefined) {
variables.add(ref)
}
OutputReference.parse(request as string).forEach((ref) => variables.add(ref))
break
}
case 'object': {
Expand Down
11 changes: 4 additions & 7 deletions tools/src/tester/StoryOutputs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import { ChapterOutput } from './ChapterOutput'
import { OutputReference } from './types/eval.types'
import { OutputReference } from './OutputReference'
import { type Parameter } from './types/story.types'

export class StoryOutputs {
Expand Down Expand Up @@ -48,12 +48,9 @@ export class StoryOutputs {
}

resolve_string (str: string): any {
const ref = OutputReference.parse(str)
if (ref) {
return this.get_output_value(ref.chapter_id, ref.output_name)
} else {
return str
}
return OutputReference.replace(str, (chapter_id, output_name) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the only place the replace function is used, then having a generic replace function that can handle any callback is a bit over-engineered, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best we keep the parsing regex values/logic in one place so it doesn't leak into the usage of it.

return this.get_output_value(chapter_id as string, output_name as string)
})
}

resolve_value (payload: any): any {
Expand Down
17 changes: 0 additions & 17 deletions tools/src/tester/types/eval.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,20 +83,3 @@ export enum Result {
SKIPPED = 'SKIPPED',
ERROR = 'ERROR',
}

export class OutputReference {
chapter_id: string
output_name: string
private constructor (chapter_id: string, output_name: string) {
this.chapter_id = chapter_id
this.output_name = output_name
}

static parse (str: string): OutputReference | undefined {
if (str.startsWith('${') && str.endsWith('}')) {
const spl = str.slice(2, -1).split('.', 2)
return { chapter_id: spl[0], output_name: spl[1] }
}
return undefined
}
}
25 changes: 24 additions & 1 deletion tools/tests/helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import _ from 'lodash'
import { delete_matching_keys, find_refs, sort_array_by_keys, to_json, to_ndjson } from '../src/helpers'
import { delete_matching_keys, find_refs, sort_array_by_keys, to_json, to_ndjson, split } from '../src/helpers'

describe('helpers', () => {
describe('sort_array_by_keys', () => {
Expand Down Expand Up @@ -195,4 +195,27 @@ describe('helpers', () => {
})).toEqual(new Set(['#1', '#2', '#3', '#dup', '#/schemas/schema1', '#/schemas/schema2']))
})
})

describe('split', () => {
test('undefined', () => {
expect(split(undefined, '.')).toEqual([])
})

test('one element', () => {
expect(split('str', '.')).toEqual(['str'])
expect(split('str', '.', -1)).toEqual(['str'])
expect(split('str', '.', 0)).toEqual(['str'])
expect(split('str', '.', 10)).toEqual(['str'])
})

test('multiple elements', () => {
expect(split('x.y.z', '.')).toEqual(['x', 'y', 'z'])
expect(split('x.y.z', '.', -1)).toEqual(['x', 'y', 'z'])
expect(split('x.y.z', '.', 0)).toEqual(['x', 'y', 'z'])
expect(split('x.y.z', '.', 1)).toEqual(['x.y.z'])
expect(split('x.y.z', '.', 2)).toEqual(['x', 'y.z'])
expect(split('x.y.z', '.', 3)).toEqual(['x', 'y', 'z'])
expect(split('x.y.z', '.', 4)).toEqual(['x', 'y', 'z'])
})
})
})
39 changes: 39 additions & 0 deletions tools/tests/tester/OutputReference.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

import { OutputReference } from "tester/OutputReference";

describe('OutputReference', () => {
let f = (id: any, k: any): string => `[${id}:${k}]`

describe('parse', () => {
it('replaces', () => {
expect(OutputReference.parse('string')).toEqual([])
expect(OutputReference.parse('${k.v}')).toEqual([new OutputReference('k', 'v')])
expect(OutputReference.parse('${k.value}')).toEqual([new OutputReference('k', 'value')])
expect(OutputReference.parse('${k.v.m}')).toEqual([new OutputReference('k', 'v.m')])
expect(OutputReference.parse('A reference to ${k.v.m} and ${x.y}.')).toEqual([
new OutputReference('k', 'v.m'),
new OutputReference('x', 'y')
])
})
})

describe('replace', () => {
it('replaces', () => {
expect(OutputReference.replace('string', f)).toEqual('string')
expect(OutputReference.replace('${k.v}', f)).toEqual('[k:v]')
expect(OutputReference.replace('${k.value}', f)).toEqual('[k:value]')
expect(OutputReference.replace('${k.v.m}', f)).toEqual('[k:v.m]')
expect(OutputReference.replace('A reference to ${k.v.m} and ${x} and ${x.y}.', f)).toEqual(
'A reference to [k:v.m] and [x:undefined] and [x:y].'
)
})
})
});
14 changes: 12 additions & 2 deletions tools/tests/tester/StoryOutputs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ const story_outputs = new StoryOutputs({

test('resolve_string', () => {
expect(story_outputs.resolve_string('${chapter_id.x}')).toEqual(1)
expect(story_outputs.resolve_string('${chapter_id.y}')).toEqual(2)
expect(story_outputs.resolve_string('${invalid_id.x}')).toBeUndefined()
expect(story_outputs.resolve_string('${invalid_id.y}')).toBeUndefined()
expect(story_outputs.resolve_string('${chapter_id.x} and ${chapter_id.y}')).toEqual('1 and 2')
expect(story_outputs.resolve_string('${chapter_id.x} and ${chapter_id.y} and ${invalid.invalid}')).toEqual('1 and 2 and undefined')
expect(story_outputs.resolve_string('some_str')).toEqual('some_str')
})

Expand Down Expand Up @@ -55,13 +59,19 @@ test('resolve_params', () => {
a: '${chapter_id.x}',
b: '${chapter_id.y}',
c: 3,
d: 'str'
d: 'str',
e: '${chapter_id.x}:${chapter_id.y}',
f: 'x=${chapter_id.x}',
g: '${chapter_id.y}=y'
}
expect(story_outputs.resolve_params(parameters)).toEqual({
a: 1,
b: 2,
c: 3,
d: 'str'
d: 'str',
e: '1:2',
f: 'x=1',
g: '2=y'
})
})

Expand Down
Loading