Create Context upcaster for complex data types

Created on 10 March 2025, about 2 months ago

Problem/Motivation

We need a means to upcast AI parameters(e.g. node:1, user:3, etc) to the expected ContextDefinition data types dynamically when setting values on FunctionCall instances.

Proposed resolution

Remaining tasks

API changes

πŸ“Œ Task
Status

Active

Version

1.1

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States michaellander

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @michaellander
  • Pipeline finished with Failed
    about 2 months ago
    Total: 427s
    #448035
  • Pipeline finished with Success
    about 2 months ago
    Total: 465s
    #448045
  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    With this branch, the workflow is:

    1. When FunctionCall::setContextValue() is called, loop through AiContextConverter plugins and check applies()
    2. If found(TRUE), stop looping and call convert()
    3. If none apply, pass original value

    Where this gets a bit tricky is to what extent applies() should evaluate in regards to returning TRUE/FALSE. Should it just match the proper format? should it be able to successfully convert without doing the conversion?

    Let's say we are given 'node:17', we can confirm the first part of the string contains a valid entity type, we can confirm the second part is a real entity. However, if entity type is correct, but the entity doesn't exist, do we return FALSE and let it continue checking for a match? One thing we may want to decide is if the applies call should remain binary or provide more potential states(think \Drupal\Core\Access\AccessResult). We'd create a AiConverterAppliesResult class, with methods such as: applicable, invalidApplicable, notApplicable. This would give us the ability to differentiate converters that definitely don't apply vs ones that may apply but theres an issue(i.e. InvalidApplicable('Node loaded but translation "fr" does not exist')) vs converters that will apply and succeed.

    This means the workflow could evolve to:

    1. When FunctionCall::setContextValue() is called, loop through AiContextConverter plugins and check applies()
    2. If applicable found, convert
    3. If invalidApplicable, return these with description on why they were invalid
    4. If notApplicable, fail, and decide what to do

    I like this approach better because it's up to the implementer to decide how to treat success, partial success and failure. They could still interpret them as true/false, or they can potentially take the natural language description and send that back to a provider and even prompt the user for clarification on how to move forward.

    I just want to maybe get some other eyes on it before I continue.

  • Pipeline finished with Success
    about 2 months ago
    Total: 205s
    #451367
  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    michaellander β†’ changed the visibility of the branch 3512100-context-upcaster_applies-class to hidden.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 230s
    #451526
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 276s
    #451549
  • Pipeline finished with Failed
    about 2 months ago
    Total: 308s
    #451554
  • πŸ‡¬πŸ‡§United Kingdom MrDaleSmith

    Failing tests.

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    I marked 'Needs review' but really looking for feedback before I go any further. I may have not used that status correctly!

  • πŸ‡¬πŸ‡§United Kingdom MrDaleSmith
  • Pipeline finished with Failed
    about 2 months ago
    Total: 417s
    #456529
  • Pipeline finished with Success
    about 2 months ago
    Total: 514s
    #456535
  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    I'm wondering if the DataType up casting/converting should be independent of Contexts...I may be unnecessarily relating them.. I'm going to go back through this and see if that's the case.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 339s
    #462684
  • Pipeline finished with Success
    about 1 month ago
    Total: 220s
    #462700
  • πŸ‡¬πŸ‡§United Kingdom MrDaleSmith

    Code all looks good an applies cleanly, but as this is such an under-the-hood change I'm finding it hard to reassure myself it has passed testing. Can you maybe suggest a test or two that would confirm it's working as expected?

  • πŸ‡©πŸ‡ͺGermany marcus_johansson

    I'm going through the AI issue queue and for me this looks good now, would you be ok with us merging it @michaellander?

Production build 0.71.5 2024