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

ET-825: Finalize Update Patient Tags AI Action #568

Merged
merged 8 commits into from
Jan 28, 2025

Conversation

belmai
Copy link
Contributor

@belmai belmai commented Jan 26, 2025

PR Type

  • Improved prompt
  • Added evaluation script (results in LangSmith
  • improved instructions for instructions :D
  • return explanations a data point
  • Removed Beta - we are confident in the action now
  • Added workaround for removing tags - need to check with the Elation team if there is a clean approach

Description

  • Enhanced updatePatientTags action by replacing "prompt" with "instructions".

  • Added an evaluation script for getTagsFromLLM function.

  • Improved tag management logic and explanations in multiple files.

  • Introduced new test cases for tag update scenarios and fixed tag clearing logic.


Changes walkthrough 📝

Relevant files
Enhancement
6 files
dataPoints.ts
Added `explanation` data point for tag updates.                   
+4/-0     
fields.ts
Replaced `prompt` with `instructions` and updated validation schema.
+5/-5     
types.ts
Updated `explanation` description in `TagsOutputSchema`. 
+1/-1     
getTagsFromLLM.ts
Replaced `prompt` with `instructions` in function parameters.
+3/-3     
prompt.ts
Refined system prompt with detailed instructions for tag
modifications.
+32/-11 
updatePatientTags.ts
Replaced prompt with instructions and added explanation to output.
+4/-3     
Tests
3 files
evaluateTags.ts
Added evaluation script for `getTagsFromLLM` function.     
+180/-0 
updatePatientTags.test.ts
Updated tests to use instructions and added explanation validation.
+6/-3     
updatePatientTagsRealOpenAI.test.ts
Added new test cases for various tag update scenarios.     
+148/-1 
Bug fix
1 files
updateTags.ts
Fixed tag clearing logic for empty arrays.                             
+3/-2     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @belmai belmai changed the title ET-825: Finalize update patient tags ai action ET-825: Finalize Update Patient Tags AI Action Jan 26, 2025
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The evaluation script and other files rely on environment variables for API keys (e.g., OPENAI_API_KEY, LANGSMITH_API_KEY). Ensure these are securely managed and not exposed in logs or error messages.

    ⚡ Recommended focus areas for review

    Manual Evaluation Script

    The evaluation script for getTagsFromLLM is designed for manual execution and does not run in CI/CD. Ensure this is acceptable and aligns with the project's testing strategy.

    /**
     * LangSmith Evaluation Script for getTagsFromLLM
     * 
     * Evaluates the getTagsFromLLM function by:
     * - Running test cases from LangSmith dataset
     * - Comparing generated tags against expected tags
     * - Generating evaluation reports
     * 
     * Requirements:
     * - Set environment variables in .env:
     *   - OPENAI_API_KEY
     *   - LANGSMITH_API_KEY
     *   - LANGSMITH_TRACING=true
     *   - LANGSMITH_PROJECT=ai-actions-local
     * 
     * Usage:
     * node getTagsFromLLM/evaluateTags.ts
     * 
     * Results can be viewed in LangSmith dashboard:
     * https://smith.langchain.com/o/3fffae83-70ff-4574-81ba-aaaedf0b4dc5/datasets/745cea13-3379-463f-9a8a-c6b10e29b8f6
     * 
     *  * ⚠️ **Note:** This script does NOT run in CI/CD. It is meant for **manual evaluation** before merging PRs (for now)
     */
    
    import { Client, type Example } from 'langsmith';
    import { evaluate } from 'langsmith/evaluation';
    import { createOpenAIModel } from '../../../../../../src/lib/llm/openai/createOpenAIModel';
    import { getTagsFromLLM } from './getTagsFromLLM';
    import { OPENAI_MODELS } from '../../../../../../src/lib/llm/openai/constants';
    import type { TagsFromAI } from './parser.js';
    import { isNil } from 'lodash';
    
    import dotenv from 'dotenv';
    dotenv.config();
    
    
    const langsmith = new Client({
      apiKey: process.env.LANGSMITH_API_KEY,
      apiUrl: process.env.LANGSMITH_ENDPOINT,
    });
    
    // Define the dataset name
    const datasetName = 'ai-actions-update-patient-tags-elation';
    
    interface EvaluatorInput {
      outputs: Record<string, unknown>;
      referenceOutputs?: Record<string, unknown>;
    }
    
    interface EvaluatorOutput {
      key: string;
      score: number;
    }
    
    interface DatasetExample {
      instruction: string;
      input_patient_tags: string[];
    }
    
    // Fetch the 'test' split examples from the dataset
    const fetchTestExamples = async (): Promise<Example[]> => {
      try {
        const testExamples = langsmith.listExamples({
          datasetName,
          splits: ['test'],
        });
        const examples: Example[] = [];
        for await (const example of testExamples) {
          if (!isNil(example.inputs) && !isNil(example.outputs)) {  // Explicit null check
            examples.push(example);
          }
        }
        return examples;
      } catch (error) {
        console.error('❌ Error fetching test examples:', error);
        throw error;
      }
    };
    
    // Custom evaluator to compare generated tags with expected tags
    const tagsMatchEvaluator = async ({
      outputs,
      referenceOutputs,
    }: EvaluatorInput): Promise<EvaluatorOutput> => {
      console.log('Evaluator received:', { outputs, referenceOutputs }); // Debug log
    
      const generatedTags = outputs?.validatedTags as string[];
      const expectedTags = referenceOutputs?.expected_updated_tags as string[];
    
      // console.log('Comparing tags:', { generatedTags, expectedTags }); // Debug log
    
      const isEqual =
        Array.isArray(generatedTags) &&
        Array.isArray(expectedTags) &&
        generatedTags.length === expectedTags.length &&
        generatedTags.every((tag, index) => tag === expectedTags[index]);
    
      return { key: 'tags_match', score: isEqual ? 1 : 0 };
    };
    
    // Wrapper function to adapt getTagsFromLLM for evaluation
    const getTagsFromLLMWrapper = async (input: DatasetExample): Promise<TagsFromAI> => {
      const payload = {
        activity: {
          id: 'test-activity-id'
        },
        pathway: {
          tenant_id: 'test-tenant',
          definition_id: 'test-definition',
          id: 'test-pathway',
          org_slug: 'test-org',
          org_id: 'test-org-id'
        },
        fields: {
          instructions: input.instruction,
          patientId: 'test-patient'
        },
        settings: {
          openAiApiKey: process.env.OPENAI_API_KEY
        }
      };
    
      const helpers = {
        getOpenAIConfig: () => {
          const apiKey = process.env.OPENAI_API_KEY;
          if (isNil(apiKey) || apiKey.trim() === '') {
            throw new Error('OPENAI_API_KEY is required but not set');
          }
          return { apiKey };
        }
      };
    
      const { model, metadata, callbacks } = await createOpenAIModel({
        settings: payload.settings,
        helpers,
        payload,
        modelType: OPENAI_MODELS.GPT4o,
      });
    
      return await getTagsFromLLM({
        model,
        existingTags: input.input_patient_tags,
        instructions: input.instruction,
        metadata,
        callbacks,
      });
    };
    
    // Main function to run the evaluation and print results
    const runEvaluation = async (): Promise<void> => {
      try {
        console.log('📡 Fetching test dataset from LangSmith...');
        const testExamples = await fetchTestExamples();
        console.log(`✅ Loaded ${testExamples.length} test examples\n`);
    
        console.log('🚀 Running evaluation...\n');
        const results = await evaluate(getTagsFromLLMWrapper, {
          data: testExamples,
          evaluators: [tagsMatchEvaluator],
          experimentPrefix: 'GetTagsFromLLM Evaluation',
          maxConcurrency: 4,
        });
    
        const resultsArray = Array.isArray(results) ? results : [results];
        const experimentId = resultsArray[0]?.run?.run_id as string;
    
        if (!isNil(experimentId) && experimentId.trim() !== '') {
          console.log('\n✨ Evaluation Complete!');
          console.log('View detailed results in LangSmith:');
          console.log(`https://smith.langchain.com/runs/${experimentId}`);
        }
    
      } catch (error) {
        console.error('❌ Error during evaluation:', error);
        throw error;
      }
    };
    
    // Execute the evaluation
    void runEvaluation();
    Tag Clearing Logic

    The logic for clearing tags by setting [''] instead of an empty array may lead to unintended behavior. Verify this approach with Elation's API documentation or support.

    export const updateElationTags = async (api: ElationAPIClient, patientId: number, tags: string[]): Promise<void> => {
        if (tags.length === 0) {
          await api.updatePatient(patientId, {
            // Note: Empty array doesn't clear tags, but [''] does by setting '' as a tag and clearing the rest
            // TODO: Check the best approach with Elation since this does not clear the tags but sets '' as a tag
            tags: [''],
          })
        } else {
          await api.updatePatient(patientId, {
            tags,
    Instructions Validation

    The instructions field requires a minimum length of 1. Confirm if additional validation (e.g., format or content checks) is necessary to prevent invalid inputs.

      instructions: {
        id: 'instructions',
        label: 'Specify tags to add, remove, or modify',
        type: FieldType.TEXT,
        required: true,
        description: 'Provide clear instructions for tag changes and relevant context, especially for uncommon tags. Specify new tags in single quotes (e.g., \'Patient-Tag\').'
      },
    } satisfies Record<string, Field>
    
    export const FieldsValidationSchema = z.object({
      patientId: NumericIdSchema,
      instructions: z.string().min(1),
    } satisfies Record<keyof typeof fields, ZodTypeAny>)

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for environment variables

    Add error handling for cases where process.env.LANGSMITH_API_KEY or
    process.env.LANGSMITH_ENDPOINT is undefined to prevent runtime errors during the
    initialization of the langsmith client.

    extensions/elation/actions/updatePatientTags/lib/getTagsFromLLM/evaluateTags.ts [37-40]

    +if (!process.env.LANGSMITH_API_KEY || !process.env.LANGSMITH_ENDPOINT) {
    +  throw new Error("LANGSMITH_API_KEY or LANGSMITH_ENDPOINT is not set in the environment variables.");
    +}
     const langsmith = new Client({
       apiKey: process.env.LANGSMITH_API_KEY,
       apiUrl: process.env.LANGSMITH_ENDPOINT,
     });
    Suggestion importance[1-10]: 9

    Why: Adding validation for environment variables ensures that the application does not fail silently or unexpectedly during runtime. This is a critical improvement for robustness and error handling.

    9
    Validate the instructions parameter

    Add validation to ensure that the instructions parameter is not empty or undefined
    before proceeding, as it is critical for generating updated tags.

    extensions/elation/actions/updatePatientTags/lib/getTagsFromLLM/getTagsFromLLM.ts [16]

    -const { model, existingTags, instructions, metadata, callbacks } = props
    +const { model, existingTags, instructions, metadata, callbacks } = props;
    +if (!instructions || instructions.trim() === '') {
    +  throw new Error("Instructions are required and cannot be empty.");
    +}
    Suggestion importance[1-10]: 9

    Why: Validating the instructions parameter is crucial as it is a required input for generating updated tags. This prevents potential runtime errors and ensures the function operates as expected.

    9
    General
    Handle invalid experiment ID gracefully

    Ensure that the evaluate function handles cases where results is not an array and
    does not contain a valid run_id, to avoid potential crashes or undefined behavior.

    extensions/elation/actions/updatePatientTags/lib/getTagsFromLLM/evaluateTags.ts [162-166]

     const resultsArray = Array.isArray(results) ? results : [results];
    -const experimentId = resultsArray[0]?.run?.run_id as string;
    +const experimentId = resultsArray[0]?.run?.run_id;
    +if (!experimentId || experimentId.trim() === '') {
    +  console.error('Experiment ID is missing or invalid.');
    +  return;
    +}
    Suggestion importance[1-10]: 8

    Why: Handling invalid experiment IDs prevents potential crashes or undefined behavior, improving the reliability and user experience of the evaluation process.

    8
    Clarify and verify tag-clearing behavior

    Clarify the behavior of the tags: [''] assignment in the comment and ensure it
    aligns with the intended functionality, as it might unintentionally set an empty
    string as a tag instead of clearing tags.

    extensions/elation/actions/updatePatientTags/updateTags.ts [6-8]

    -// Note: Empty array doesn't clear tags, but [''] does by setting '' as a tag and clearing the rest
    +// Note: Empty array doesn't clear tags. [''] sets an empty string as a tag, which might not be the intended behavior. Verify with Elation API for proper tag clearing.
     tags: [''],
    Suggestion importance[1-10]: 7

    Why: The clarification helps developers understand the behavior of the code and ensures that the functionality aligns with the intended purpose. However, it does not directly fix an issue, so the impact is moderate.

    7

    @belmai belmai requested a review from nckhell January 26, 2025 22:46
    @belmai belmai requested a review from nckhell January 28, 2025 08:10
    @belmai belmai self-assigned this Jan 28, 2025
    @belmai
    Copy link
    Contributor Author

    belmai commented Jan 28, 2025

    I am merging...

    @belmai belmai merged commit b036e57 into main Jan 28, 2025
    2 checks passed
    @belmai belmai deleted the et-825-finalize-update-patient-tags-ai-action branch January 28, 2025 12:54
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants