Skip to content

Conversation

lrafeei
Copy link
Contributor

@lrafeei lrafeei commented Nov 3, 2023

Overview

Modify get_ai_message_ids API function to retrieve message IDs to be used in the customer's AI feedback endpoint.

umaannamalai and others added 27 commits October 13, 2023 11:18
commit 2834663
Author: Timothy Pansino <[email protected]>
Date:   Mon Oct 9 17:42:05 2023 -0700

    OpenAI Mock Backend (#929)

    * Add mock external openai server

    * Add mocked OpenAI server fixtures

    * Set up recorded responses.

    * Clean mock server to depend on http server

    * Linting

    * Pin flask version for flask restx tests. (#931)

    * Ignore new redis methods. (#932)

    Co-authored-by: Lalleh Rafeei <[email protected]>

    * Remove approved paths

    * Update CI Image (#930)

    * Update available python versions in CI

    * Update makefile with overrides

    * Fix default branch detection for arm builds

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

    * Add mocking for embedding endpoint

    * [Mega-Linter] Apply linters fixes

    * Add ratelimit headers

    * [Mega-Linter] Apply linters fixes

    * Only get package version once (#928)

    * Only get package version once

    * Add disconnect method

    * Add disconnect method

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

    * Add datalib dependency for embedding testing.

    * Add OpenAI Test Infrastructure (#926)

    * Add openai to tox

    * Add OpenAI test files.

    * Add test functions.

    * [Mega-Linter] Apply linters fixes

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
    Co-authored-by: mergify[bot] <mergify[bot]@users.noreply.github.com>

    * Add mock external openai server

    * Add mocked OpenAI server fixtures

    * Set up recorded responses.

    * Clean mock server to depend on http server

    * Linting

    * Remove approved paths

    * Add mocking for embedding endpoint

    * [Mega-Linter] Apply linters fixes

    * Add ratelimit headers

    * [Mega-Linter] Apply linters fixes

    * Add datalib dependency for embedding testing.

    ---------

    Co-authored-by: Uma Annamalai <[email protected]>
    Co-authored-by: Lalleh Rafeei <[email protected]>
    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
    Co-authored-by: TimPansino <[email protected]>
    Co-authored-by: Hannah Stepanek <[email protected]>
    Co-authored-by: mergify[bot] <mergify[bot]@users.noreply.github.com>

commit db63d45
Author: Uma Annamalai <[email protected]>
Date:   Mon Oct 2 15:31:38 2023 -0700

    Add OpenAI Test Infrastructure (#926)

    * Add openai to tox

    * Add OpenAI test files.

    * Add test functions.

    * [Mega-Linter] Apply linters fixes

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
    Co-authored-by: mergify[bot] <mergify[bot]@users.noreply.github.com>
commit 182c7a8
Author: Uma Annamalai <[email protected]>
Date:   Fri Oct 13 10:12:55 2023 -0700

    Add request/ response IDs.

commit f6d13f8
Author: Uma Annamalai <[email protected]>
Date:   Thu Oct 12 13:23:39 2023 -0700

    Test cleanup.

commit d057663
Author: Uma Annamalai <[email protected]>
Date:   Tue Oct 10 10:23:00 2023 -0700

    Remove commented code.

commit dd29433
Author: Uma Annamalai <[email protected]>
Date:   Tue Oct 10 10:19:01 2023 -0700

    Add openai sync instrumentation.

commit 2834663
Author: Timothy Pansino <[email protected]>
Date:   Mon Oct 9 17:42:05 2023 -0700

    OpenAI Mock Backend (#929)

    * Add mock external openai server

    * Add mocked OpenAI server fixtures

    * Set up recorded responses.

    * Clean mock server to depend on http server

    * Linting

    * Pin flask version for flask restx tests. (#931)

    * Ignore new redis methods. (#932)

    Co-authored-by: Lalleh Rafeei <[email protected]>

    * Remove approved paths

    * Update CI Image (#930)

    * Update available python versions in CI

    * Update makefile with overrides

    * Fix default branch detection for arm builds

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

    * Add mocking for embedding endpoint

    * [Mega-Linter] Apply linters fixes

    * Add ratelimit headers

    * [Mega-Linter] Apply linters fixes

    * Only get package version once (#928)

    * Only get package version once

    * Add disconnect method

    * Add disconnect method

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

    * Add datalib dependency for embedding testing.

    * Add OpenAI Test Infrastructure (#926)

    * Add openai to tox

    * Add OpenAI test files.

    * Add test functions.

    * [Mega-Linter] Apply linters fixes

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
    Co-authored-by: mergify[bot] <mergify[bot]@users.noreply.github.com>

    * Add mock external openai server

    * Add mocked OpenAI server fixtures

    * Set up recorded responses.

    * Clean mock server to depend on http server

    * Linting

    * Remove approved paths

    * Add mocking for embedding endpoint

    * [Mega-Linter] Apply linters fixes

    * Add ratelimit headers

    * [Mega-Linter] Apply linters fixes

    * Add datalib dependency for embedding testing.

    ---------

    Co-authored-by: Uma Annamalai <[email protected]>
    Co-authored-by: Lalleh Rafeei <[email protected]>
    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
    Co-authored-by: TimPansino <[email protected]>
    Co-authored-by: Hannah Stepanek <[email protected]>
    Co-authored-by: mergify[bot] <mergify[bot]@users.noreply.github.com>

commit db63d45
Author: Uma Annamalai <[email protected]>
Date:   Mon Oct 2 15:31:38 2023 -0700

    Add OpenAI Test Infrastructure (#926)

    * Add openai to tox

    * Add OpenAI test files.

    * Add test functions.

    * [Mega-Linter] Apply linters fixes

    ---------

    Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
    Co-authored-by: mergify[bot] <mergify[bot]@users.noreply.github.com>
* Cache _get_package_version

* Add Python 2.7 support to get_package_version caching

* [Mega-Linter] Apply linters fixes

* Bump tests

---------

Co-authored-by: SlavaSkvortsov <[email protected]>
Co-authored-by: TimPansino <[email protected]>
* Fix scan_iter for redis

* Replace generator methods

* Update instance info instrumentation

* Remove mistake from uninstrumented methods

* Add skip condition to asyncio generator tests

* Add skip condition to asyncio generator tests

---------

Co-authored-by: Lalleh Rafeei <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Checkout old action

* Adding RPM action

* Add dry run

* Incorporating action into workflow

* Wire secret into custom action

* Enable action

* Correct action name

* Fix syntax

* Fix quoting issues

* Drop pre-verification. Does not work on python

* Fix merge artifact
Co-authored-by: Lalleh Rafeei <[email protected]>
Co-authored-by: Lalleh Rafeei <[email protected]>
Co-authored-by: Hannah Stepanek <[email protected]>
@lrafeei lrafeei changed the base branch from main to develop-bedrock-instrumentation November 3, 2023 21:08
Copy link

github-actions bot commented Nov 3, 2023

🦙 MegaLinter status: ❌ ERROR

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON bandit 3 0 4.94s
✅ PYTHON black 5 2 0 1.25s
❌ PYTHON flake8 5 1 0.49s
✅ PYTHON isort 5 2 0 0.23s
✅ PYTHON pylint 5 0 3.91s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@mergify mergify bot added the tests-failing Tests failing in CI. label Nov 3, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop-bedrock-instrumentation@0882ba4). Click here to learn what that means.
The diff coverage is n/a.

@@                        Coverage Diff                         @@
##             develop-bedrock-instrumentation     #958   +/-   ##
==================================================================
  Coverage                                   ?   81.90%           
==================================================================
  Files                                      ?      190           
  Lines                                      ?    19767           
  Branches                                   ?     3436           
==================================================================
  Hits                                       ?    16190           
  Misses                                     ?     2590           
  Partials                                   ?      987           

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@mergify mergify bot added the merge-conflicts Merge conflicts detected. label Nov 6, 2023
@mergify mergify bot removed the merge-conflicts Merge conflicts detected. label Nov 6, 2023
Copy link
Contributor

@hmstepanek hmstepanek left a comment

Choose a reason for hiding this comment

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

I had a couple changes. Primarily, we are missing the request_id in the tuple of ids returned when get_ai_message_ids is called.

I had some additional feedback about the tests to make sure we are covering everything.

@hmstepanek hmstepanek marked this pull request as ready for review November 6, 2023 23:10
@hmstepanek hmstepanek requested a review from a team November 6, 2023 23:10
@mergify mergify bot removed the tests-failing Tests failing in CI. label Nov 8, 2023
@lrafeei lrafeei requested a review from hmstepanek November 8, 2023 20:43
Copy link
Contributor

@hmstepanek hmstepanek left a comment

Choose a reason for hiding this comment

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

In Bedrock the customer is never required to provide the response_id to get the message ids so the requested changes are centered around that. The response_id is required for OpenAI since OpenAI has async capabilities and we need to be able to distinguish one call's message ids from another's (since you could have multiple async calls before calling get_ai_message_ids). This is not the case in Bedrock since it doesn't support async so the customer doesn't have to pass an identifier in for us to know which bedrock call they want message ids for.

We recently decided to change the name of this public api function to get_llm_message_ids so while you're at it if you wouldn't mind updating the name in your PR, Uma is taking care of the rest of the name changes in her PR to openai.

Otherwise everything else looks good.

@mergify mergify bot added the tests-failing Tests failing in CI. label Nov 9, 2023
@mergify mergify bot added merge-conflicts Merge conflicts detected. and removed tests-failing Tests failing in CI. labels Nov 9, 2023
@lrafeei lrafeei requested a review from hmstepanek November 9, 2023 21:54
@mergify mergify bot added tests-failing Tests failing in CI. and removed merge-conflicts Merge conflicts detected. labels Nov 9, 2023
@mergify mergify bot removed the tests-failing Tests failing in CI. label Nov 9, 2023
Copy link
Contributor

@hmstepanek hmstepanek left a comment

Choose a reason for hiding this comment

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

Looks great!

@hmstepanek hmstepanek merged commit ef2f39c into develop-bedrock-instrumentation Nov 10, 2023
@hmstepanek hmstepanek deleted the add-bedrock-feedback branch November 10, 2023 00:07
@umaannamalai umaannamalai added this to the v9.8.0 milestone Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants