Skip to content

CV2-5077-Add-test-coverage-for-OpenAI-module-in-Alegre #491

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Feb 28, 2025
Merged

CV2-5077-Add-test-coverage-for-OpenAI-module-in-Alegre #491

merged 15 commits into from
Feb 28, 2025

Conversation

ahmednasserswe
Copy link
Contributor

Description

to add test coverage for openai.py

Reference: CV2-5077

skyemeedan
skyemeedan reviewed Feb 13, 2025
View reviewed changes
Copy link
Contributor

@skyemeedan skyemeedan left a comment

Choose a reason for hiding this comment

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

Thanks Ahmed! I think this is a good first pass of making sure our code for interacting with OpenAI is callable. However, as we were discussing in the chat, I don't think it helps us nail down the behavior of the library or ensure that it will actually work. i.e. I think these tests would still pass if OpenAI went away. Can you create a separate "integration test" that validates that we are able to actually call the system? Maybe the tests for the google langid stuff could be a template:

alegre/app/test/test_translation.py

Line 9 in 41c9f8e

from app.main.lib.google_client import get_credentialed_google_client

I think this is especially important, because it looks like the function in the OpenAI python library we are using may have been removed openai/openai-python#676 so we will need to know when upgrading our open AI wrapper will break the feature.

computermacgyver
computermacgyver reviewed Feb 13, 2025
View reviewed changes
computermacgyver
computermacgyver reviewed Feb 18, 2025
View reviewed changes
@skyemeedan skyemeedan requested a review from dmou as a code owner February 27, 2025 02:02
@skyemeedan skyemeedan self-requested a review February 28, 2025 05:54
skyemeedan
skyemeedan approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@skyemeedan skyemeedan left a comment

Choose a reason for hiding this comment

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

Seems like the testing issues are resolved by adding the test openai key to the encrypted env variables, so this is ready to merge!

@skyemeedan skyemeedan merged commit 008f4f2 into develop Feb 28, 2025
4 checks passed
skyemeedan pushed a commit that referenced this pull request Mar 3, 2025
v0.187.2 Squashed commit of the following from develop:
4eb96c4 
commit 008f4f2
Author: ahmednasserswe <ahmed.nasser.swe@gmail.com>
Date:   Fri Feb 28 06:58:11 2025 +0100

    CV2-5077-Add-test-coverage-for-OpenAI-module-in-Alegre (#491)

    * create test_openai.py and add `test_openai_get_document_body`
    * Update test_openai.py with `test_retrieve_openai_embeddings_handles_api_error` and`test_retrieve_openai_embeddings_calls_openai_api`
    * Update test_openai.py to mock redis cache
    * Update test_openai.py to include tests that call the api
    * mocking cache in `test_openai_get_document_body` and `test_retrieve_openai_embeddings_calls_openai_api`
    * asserting openai key is set during CI
    * adding a unit test to find if openai_api_key is in env
    * added test OPENAI key for CI to encoded .env file https://meedan.atlassian.net/wiki/spaces/ENG/pages/1942945794/How+to+encrypt+and+decrypt+.enc+files+in+Alegre

    ---------

    Co-authored-by: Skye Bender-deMoll <skye@meedan.org>

commit d63cc85
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Thu Feb 27 15:48:30 2025 -0800

    update to show checkweb direct dependency on checkApi

commit 60be6ac
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Wed Feb 26 14:39:37 2025 -0800

    added links to new docs in README, moved png files from toplevel into /doc

commit 3636375
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Wed Feb 26 14:28:44 2025 -0800

    added explanation and API dependencies

commit cb7e5b1
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Wed Feb 26 14:22:51 2025 -0800

    renamed and corrected image links

commit 9412b7a
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Wed Feb 26 12:46:38 2025 -0800

    [CV2-5885] first sketch of infra based on notes from Devin

commit 7b135a2
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Thu Feb 27 16:16:11 2025 -0800

    adding --threads back in

commit 4ea58a1
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Thu Feb 27 15:09:00 2025 -0800

    [CV2-6066] doubling worker processes from 8 to 16, and removing --threads since docs show it is only for gevent

commit 94dd470
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Thu Feb 27 11:12:59 2025 -0800

    doubling threads again from 8 to 16

commit 227fa79
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Thu Feb 27 11:11:42 2025 -0800

    disabling some tests to simplify testing protocol

commit 1afea6a
Author: Skye Bender-deMoll <skye@meedan.org>
Date:   Thu Feb 27 11:03:58 2025 -0800

    [CV2-6066] added a load test for async endpoint

commit cf3e2c2
Author: Martin Peck <51542678+sonoransun@users.noreply.github.com>
Date:   Wed Feb 26 08:25:46 2025 -0700

    Fix ecs deploy dependencies with updated version pins. (#493)

    * Try to resolve dependency errors in ecs deploy by updating botocore, boto3, and awscli version pins.

commit c0930a9
Author: Devin Gaffney <itsme@devingaffney.com>
Date:   Tue Feb 25 20:10:14 2025 -0500

    CV2-6022 update tests to use non-deprecated URLs (#487)

    * CV2-6022 update tests to use non-deprecated (presto backed) URLs
    * update queries to no longer use old endpoint
    * test for min_es_score fix
    * add custom error
    * add sleeps, it ended up just being eventual consistency
    * switch to greaters, deprecated test file having weird interaction effects that we probably don't need to solve for right now
    * Update app/test/test_similarity.py

commit 6645936
Author: Skye Bender-deMoll <122867176+skyemeedan@users.noreply.github.com>
Date:   Tue Feb 25 11:00:33 2025 -0800

    [CV2-6066] load tests 3 (#492)

    * [CV2-6066] change warning message to not include actual key value so sentry errors will group
    * double number of alegre web server worker process-THREADS to 8 in QA and Live to handle more connections
    ---------
    Co-authored-by: Skye Bender-deMoll <skye@meedan.org>

commit c4828f5
Author: Devin Gaffney <itsme@devingaffney.com>
Date:   Mon Feb 24 14:32:45 2025 -0800

    [CV2-6075] documentation for Check-Alegre-Presto media indexing and similarity workflows (#489)

    * CV2-6075 initial stab at documentation for Check-Alegre-Presto media indexing and similarity workflows

commit 3494d9e
Author: Skye Bender-deMoll <122867176+skyemeedan@users.noreply.github.com>
Date:   Fri Feb 14 09:06:34 2025 -0800

    [CV2-6066] load tests for text similarity endpoint (#490)

    * adding faker pip requirement for random text generation for benchmarks (testing only)
    * [CV2-6066] draft load testing script for text similarity endpoints
    * updates to benchmark (disabling parallel inserts) and some code comments
    * re-enabled parallel tests, reducing so all tests only run 100 items
    * added check to not run in live
    ---------
    Co-authored-by: Skye Bender-deMoll <skye@meedan.org>

commit 41c9f8e
Author: Skye Bender-deMoll <122867176+skyemeedan@users.noreply.github.com>
Date:   Fri Feb 7 08:45:44 2025 -0800

    [CV2-6020] test updates and handling for missing min_es_score (#485)

    * [CV2-6020] test updates and handling for missing min_es_score
    * added more explicit error messaging and confirmed error status code, corrected failing test
    ---------
    Co-authored-by: Skye Bender-deMoll <skye@meedan.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@computermacgyver computermacgyver computermacgyver left review comments

@DGaffney DGaffney Awaiting requested review from DGaffney

@dmou dmou Awaiting requested review from dmou

+1 more reviewer

@skyemeedan skyemeedan skyemeedan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ahmednasserswe @computermacgyver @skyemeedan