feat: REL-10772 - Part 1 - Added server-ai core packages #65
Conversation
… metrics tracking.
edwinokonkwo
changed the title
jsonbailey
left a comment
jsonbailey
left a comment
There was a problem hiding this comment.
This still has the old files in the old location. I would either make these changes to the old file, and then do the move. Or do the move in this pr by deleting the old files. That gives git a chance to recognize the same files and show the proper diff.
jsonbailey
left a comment
jsonbailey
left a comment
There was a problem hiding this comment.
There are a few changes that need to be made for the project structure. You still may find it easier to separate the reorganization of the package into a separate commit from the addition of the Judges and Chat implementations.
| @@ -0,0 +1,573 @@ | |||
| import asyncio | |||
There was a problem hiding this comment.
It doesn't look like git is properly tracking this as a moved file, rather it thinks it was deleted and a new file created. We should ensure this shows as moved so that the history still works on the file.
| steps: | ||
| - name: Build distribution files | ||
| shell: bash | ||
| working-directory: packages/sdk/server-ai |
There was a problem hiding this comment.
There are a couple options here. The js-core repository passes in the workspace directory and has individual CI build workflows for each project (https://github.com/launchdarkly/js-core/blob/main/.github/workflows/release-please.yml).
The dotnet repository uses shared workflows but sets a github_actions.env file for each package that is loaded during the CI / build process https://github.com/launchdarkly/dotnet-core/tree/main/pkgs/sdk/server-ai.
| ) | ||
|
|
||
| if eval_result and eval_result.success: | ||
| self._tracker.track_eval_scores(eval_result.evals) |
There was a problem hiding this comment.
The js-core repo switched to using trackJudgeResponse vs eval scores as it contained information about the judge ai config key. We should make that change here as well.
| from ldai.tracker import LDAIConfigTracker | ||
|
|
||
|
|
||
| class AIJudge: |
There was a problem hiding this comment.
I think we can just call this Judge vs AIJudge to keep it aligned with js-core. Or we might consider updating js-core.
| return list(provider_set) # type: ignore[arg-type] | ||
|
|
||
| @staticmethod | ||
| async def _try_create_provider( |
There was a problem hiding this comment.
This logic is only needed if we cannot do true dynamic imports. JS had issues with that so we needed to use a switch and declare each import so that tree shaking didn't remove the package but python may not have that issue. This code has two approaches and we should test if dynamic works and if so, use it. Otherwise do the direct imports.
This logic for direct imports was simplified in the latest js-core and would likely work here as well. https://github.com/launchdarkly/js-core/blob/main/packages/sdk/server-ai/src/api/providers/AIProviderFactory.ts#L96
packages/sdk/server-ai/src/ldai/providers/ai_provider_factory.py
Outdated
Show resolved
Hide resolved
| ) | ||
|
|
||
| def track_openai_metrics(self, func): | ||
| async def track_openai_metrics(self, func): |
There was a problem hiding this comment.
Why is this method being modified?
|
|
||
|
|
||
| def test_tracks_openai_metrics(client: LDClient): | ||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
Same here, I'm not sure why we made this async. It seems unrelated to the current PR.
| @@ -0,0 +1,13 @@ | |||
| Copyright 2024 Catamorphic, Co. | |||
There was a problem hiding this comment.
I don't think this needs to be duplicated and should stay in the root unless we need it for the packaging process.
| @@ -0,0 +1,41 @@ | |||
| # LaunchDarkly Server-side AI library for Python | |||
There was a problem hiding this comment.
Same here, we have just duplicated this file but left the original at the root. This make sense to move, but we should change the root README in that case.
Part 1: Add server-ai package
We initially reviewed this as a single PR, but decided to align the folder structure with js-core for consistency.
This PR introduces the server-ai package, setting up the foundation for the new structure.
Part 1: Adds the server-ai package (this PR)
Part 2: Adds the LangChain provider ( next )
Part 3: Updates CI and publishing configuration ( next )