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 AI variable dependencies explicitly #15000

Open
Tracked by #15068
planger opened this issue Feb 21, 2025 · 3 comments
Open
Tracked by #15068

Support AI variable dependencies explicitly #15000

planger opened this issue Feb 21, 2025 · 3 comments
Assignees
Labels

Comments

@planger
Copy link
Contributor

planger commented Feb 21, 2025

In the course of #14971, we identified the potential need of AI variables that depend on other variables. The concrete example use case was a chat context summary variable that lists all variables of the context; to be able to summarize a context variable, it needs to be resolved first. Other use cases may involve reusing core variables, like workspace root, environment, etc.

Currently, resolving a variable in the course of another variable resolution is not explicitly supported; the variable provider can only capture a reference to the variable service when it is asked to register the variable itself, and then use this variable. It cannot get the variable service injected directly, as this would cause cyclic dependency injection. Also we may run into cyclic variable resolutions.

As a potential solution we discussed the approach below.

Extend the variable resolver

export interface AIVariableResolverWithVariableDependencies extends AIVariableResolver {
    resolve(
        request: AIVariableResolutionRequest,
        context: AIVariableContext,
        resolveDependency: (req: AIVariableResolutionRequest) => Promise<ResolvedAIVariable | undefined>
    ): Promise<ResolvedAIVariable | undefined>;
}

function isResolverWithDependencies(resolver: AIVariableResolver | undefined): resolver is AIVariableResolverWithVariableDependencies {
    return resolver !== undefined && resolver.resolve.length >= 3;
}

Enhance the resolveVariable method of the variable service

interface CacheEntry {
    promise: Promise<ResolvedAIVariable | undefined>;
    inProgress: boolean;
}

export class DefaultAIVariableService implements AIVariableService {
    // ... existing properties and methods

    async resolveVariable(
        request: AIVariableArg,
        context: AIVariableContext,
        cache: Map<string, CacheEntry> = new Map()
    ): Promise<ResolvedAIVariable | undefined> {
        const variableName = typeof request === 'string'
            ? request
            : typeof request.variable === 'string'
                ? request.variable
                : request.variable.name;

        if (cache.has(variableName)) {
            const entry = cache.get(variableName)!;
            if (entry.inProgress) {
                this.logger.warn(`Cycle detected for variable: ${variableName}. Skipping resolution.`);
                return undefined;
            }
            return entry.promise;
        }

        const entry: CacheEntry = { promise: Promise.resolve(undefined), inProgress: true };
        cache.set(variableName, entry);

        const promise = (async () => {
            const variable = this.getVariable(variableName);
            if (!variable) {
                return undefined;
            }
            const arg = typeof request === 'string' ? undefined : request.arg;
            const resolver = await this.getResolver(variableName, arg, context);
            let resolved: ResolvedAIVariable | undefined;
            if (isResolverWithDependencies(resolver)) {
                resolved = await resolver.resolve(
                    { variable, arg },
                    context,
                    async (depRequest: AIVariableResolutionRequest) =>
                        this.resolveVariable(depRequest, context, cache)
                );
            } else {
                resolved = await resolver?.resolve({ variable, arg }, context);
            }
            return resolved ? { ...resolved, arg } : undefined;
        })();

        entry.promise = promise;
        promise.finally(() => {
            entry.inProgress = false;
        });

        return promise;
    }
}

Summary

  • AIVariableResolverWithVariableDependencies lets a resolver access dependencies through a callback during its resolution.
  • The updated resolveVariable method uses a promise-based cache with an inProgress flag for cycle detection. This way, if a variable is already being resolved, it’s skipped—avoiding cycles.
  • With this approach, resolved variables are accessible during the resolution of dependent ones.
@lucas-koehler
Copy link
Contributor

@planger As I see it the current suggestion aborts with a cycle detection if the same variable is encountered again with a different argument. I.e. only the variable names are considered:

const variableName = typeof request === 'string'
    ? request
    : typeof request.variable === 'string'
        ? request.variable
        : request.variable.name;

if (cache.has(variableName)) {
    const entry = cache.get(variableName)!;
    if (entry.inProgress) {
        this.logger.warn(`Cycle detected for variable: ${variableName}. Skipping resolution.`);
        return undefined;
    }
    return entry.promise;
}

I think this could be too strict. As an example prompt fragments come to mind: They will all be referenced via variable prompt. However, fragment foo (variable prompt:foo) should be able to reference another fragment bar (variable prompt:bar).

Thus, I think we need to consider the combination of variable and argument instead of just the variable for cycle detection. What do you think?

@planger
Copy link
Contributor Author

planger commented Feb 27, 2025

Absolutely right, it needs to take the arguments into account. Good catch!

@lucas-koehler lucas-koehler self-assigned this Feb 27, 2025
@lucas-koehler
Copy link
Contributor

Planned approach to return recursively resolved variables as part of the returned data of resolveVariable:
Extend ResolvedAiVariable with a prop allResolvedDependencies: ResolvedAIVariable[] that contains a flat list of all recursively resolved dependencies.
This way, consumers can easily check which variables where recursively resolved after resolving a variable. Consumer only interested in the resolved text can do this as before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants