feat(backend): refactor how inference providers are added and configured #475

AgustinRamiroDiaz · 2024-09-05T11:14:43Z

Fixes #359

What

Added configuration for LLM providers
We now have a schema defined in https://json-schema.org/, which checks that LLM provider configurations are compliant.

Expected flow for new providers

Extend the JSON schema to add their providers
If not compatible with other plugins, add a plugin to llms.py
add a migration to load these new providers into the database

Step 3 seems to be the hardest for newcomers, since it would require a guide to use Alembic and SQLAlchemy to fill the database. Also, step 3 could not be desired since we'd be modifying provider configurations that users could've tuned and maybe don't want to change.
I'm open to ideas

Why

To allow new OpenAI compliant providers to come in and add their own by self registering

Testing done

Created e2e tests and unit tests

Decisions made

Decided to use json schema since
- it's a big standard with an ecosystem of tools
- JSON is widely adopted, like in our JSON-rpc
I've made a lot of extras along the way with the intent of improving the codebase as I was passing by

Checks

I have tested this code
I have reviewed my own PR
I have created an issue for this PR
I have set a descriptive PR title compliant with conventional commits

Reviewing tips

Don't check git history
review in order:
1. json-schema
2. tests
3. code

User facing release notes

Refactored inference providers: now LLM providers and models can be extended and configured easily, opening the door for new providers to register.
Added new OpenAI o1-mini and o1-preview models
Added plugin to allow using Anthropic (Claude)

AgustinRamiroDiaz · 2024-09-06T22:37:35Z

.pre-commit-config.yaml

@@ -13,7 +13,7 @@ repos:
    hooks:
      - id: backend-unit-pytest
        name: backend unit tests with pytest
-        entry: python3 -m pytest backend
+        entry: pytest tests/unit


extra: moved unit tests

AgustinRamiroDiaz · 2024-09-06T22:37:58Z

.vscode/settings.json

+  "python.testing.pytestArgs": ["tests", "backend"],
  "python.testing.unittestEnabled": false,
-  "python.testing.pytestEnabled": true
+  "python.testing.pytestEnabled": true,
+  "sonarlint.connectedMode.project": {
+    "connectionId": "YeagerAI",
+    "projectKey": "yeagerai_genlayer-simulator"
+  }
 }


extra: pytest and sonar vscode config

AgustinRamiroDiaz · 2024-09-06T22:38:16Z

backend/database_handler/alembic.ini

@@ -13,7 +13,7 @@ script_location = migration

 # sys.path path, will be prepended to sys.path if present.
 # defaults to the current working directory.
-prepend_sys_path = .
+prepend_sys_path = ./backend/database_handler


needed for imports

AgustinRamiroDiaz · 2024-09-06T22:39:11Z

backend/database_handler/llm_providers.py

+    def get_all_dict(self) -> list[dict]:
+        return [
+            {
+                "id": provider.id,
+                "provider": provider.provider,
+                "model": provider.model,
+                "config": provider.config,
+            }
+            for provider in self.session.query(LLMProviderDBModel).all()
+        ]


this is duplicated since LLMProvider does not have an ID. I think that it's fine for now, but I'm open to opinions on "should the domain models have IDs?"

AgustinRamiroDiaz · 2024-09-06T22:41:23Z

backend/database_handler/models.py

+    provider: Mapped[str] = mapped_column(String(255))
+    model: Mapped[str] = mapped_column(String(255))


I did not add this pair of (provider, model) as a unique key since I feel that we'd be limiting the users. I think that it makes sense that users can create many provider configurations, and then use that pool to draw from

AgustinRamiroDiaz · 2024-09-06T22:42:58Z

backend/database_handler/models.py

@@ -110,3 +110,22 @@ class Validators(Base):
    created_at: Mapped[Optional[datetime.datetime]] = mapped_column(
        DateTime(True), server_default=func.current_timestamp(), init=False
    )
+
+
+class LLMProviderDBModel(Base):


One question that I'm thinking now is that if this should have any relationship with the validators, or if this is a pool to draw configurations with that could live independently?

The second one.
Could you please clarify the name? If the providers are called LLMProvider, why isn't this one called LLMProviderModel or just LLMModel?

I was trying to come up with a convention around naming for the database models (the ones that are used with SQLAlchemy), so I thought about adding DBModel to it. I now see it's confusing since we also have the model from the llm.

Maybe just LLMProviderDB?

backend/protocol_rpc/endpoints.py

AgustinRamiroDiaz · 2024-09-06T22:45:37Z

docker/Dockerfile.backend

@@ -25,7 +25,7 @@ COPY backend $path/backend
 FROM base AS debug
 RUN pip install --no-cache-dir debugpy watchdog
 USER backend-user
-CMD watchmedo auto-restart --recursive --pattern="*.py" --ignore-patterns="*.pyc;*__pycache__*" -- python -m debugpy --listen 0.0.0.0:${RPCDEBUGPORT} -m flask run -h 0.0.0.0 -p ${FLASK_SERVER_PORT}
+CMD watchmedo auto-restart --no-restart-on-command-exit --recursive --pattern="*.py" --ignore-patterns="*.pyc;*__pycache__*" -- python -m debugpy --listen 0.0.0.0:${RPCDEBUGPORT} -m flask run -h 0.0.0.0 -p ${FLASK_SERVER_PORT}


extra: doesn't restart when code exits, which most of the time is when there are "compile" errors

cristiam86

I like the approach and it is a big improvement from what we have now. Still, we need to try to document and follow the process of adding a new provider and model ourselves to identify some potential problems that anyone could have.
What concerns me more is the fact that a new provider needs to be added in different places because we have the default configs, some enums in the schemas, the database entry, etc.
Maybe the ideal scenario where someone just adds providers and models from the UI is very hard to achieve but we need to try to be closer to that one.

cristiam86 · 2024-09-10T07:49:00Z

backend/node/create_nodes/providers.py

+
+    return_value = []
+
+    with warnings.catch_warnings():


@AgustinRamiroDiaz could you please add some comments to understand this solution better?

backend/protocol_rpc/endpoints.py

cristiam86 · 2024-09-10T07:54:58Z

backend/protocol_rpc/endpoints.py

+        LLMProvider(
+            provider=provider,
+            model=model,
+            config=config,


@AgustinRamiroDiaz, should we load the default one if the user sends an empty config?

@cristiam86 this comment got moved and it's not pointing to the code you mention. Could you remind me on which function is this?

tests/integration/conftest.py

cristiam86 · 2024-09-10T08:00:09Z

tests/integration/test_llm_providers_registry.py

+    first_default_provider = default_providers[0]
+    last_provider_id = default_providers[-1]["id"]
+
+    # Create a new provider


@AgustinRamiroDiaz this seems too complicated and with some intrinsic logic. Could we just add manual hardcoded values?

I didn't wanted to add hardcoded values since they'd be changing with the schema, but you are right. Once we have a stable schema these tests shouldn't change often

I'll create another test (I think this one adds it's own logic so I won't remove it)

cristiam86 · 2024-09-10T08:04:26Z

tests/integration/test_validators.py

@@ -0,0 +1,44 @@
+from tests.common.request import payload, post_request_localhost


@AgustinRamiroDiaz shouldn't we add a test for the update_validator?

tests/unit/test_providers.py

The base branch was changed.

codecov · 2024-09-11T17:50:17Z

Codecov Report

Attention: Patch coverage is 65.51724% with 10 lines in your changes missing coverage. Please review.

Project coverage is 17.35%. Comparing base (c4c2d77) to head (a16920c).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
frontend/src/stores/node.ts	36.36%	7 Missing ⚠️
frontend/src/services/JsonRpcService.ts	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #475      +/-   ##
==========================================
- Coverage   17.90%   17.35%   -0.55%     
==========================================
  Files         111      111              
  Lines        7955     7928      -27     
  Branches      191      189       -2     
==========================================
- Hits         1424     1376      -48     
- Misses       6456     6477      +21     
  Partials       75       75

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz · 2024-09-12T14:29:17Z

backend/database_handler/migration/versions/986d9a6b0dda_add_plugin_and_plugin_config_to_.py

+            .values(
+                plugin=default_provider.plugin,
+                plugin_config=default_provider.plugin_config,
+                config=default_provider.config,


Note that we are overriding existing configs. This is because the schema has changed and it's the simplest way. Handling the logic of migrating current configs is possible, but would be a lot of work and it's probably not a requirement

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz · 2024-09-13T15:20:48Z

@cristiam86 I've finished the PR and it's now ready to be reviewed

Note that I haven't been able to test the Claude Plugin

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

cristiam86

Good job @AgustinRamiroDiaz. I have one concern about the performance adding/updating validators which now takes ~1 second while before it was instant. Do you happen to know why? Could it be related to the way we load the JSON providers files? could we paralelize that somehow?

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz · 2024-09-16T18:31:07Z

@cristiam86 you were right! Fixed in my last commit by adding a cache + running it at startup 👍

github-actions · 2024-09-18T08:03:10Z

🎉 This PR is included in version 0.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

AgustinRamiroDiaz force-pushed the 359-sim-refactor-how-inference-providers-are-added-and-configured branch from 5e6824c to b012247 Compare September 6, 2024 22:28

AgustinRamiroDiaz changed the title ~~359 sim refactor how inference providers are added and configured~~ feat(backend): refactor how inference providers are added and configured Sep 6, 2024

AgustinRamiroDiaz commented Sep 6, 2024

View reviewed changes

AgustinRamiroDiaz marked this pull request as ready for review September 6, 2024 22:46

AgustinRamiroDiaz added the run-tests label Sep 6, 2024

AgustinRamiroDiaz requested a review from cristiam86 September 9, 2024 10:53

AgustinRamiroDiaz removed the run-tests label Sep 9, 2024

AgustinRamiroDiaz force-pushed the 359-sim-refactor-how-inference-providers-are-added-and-configured branch from 13dca11 to 4d0ba72 Compare September 9, 2024 18:07

denishacquin mentioned this pull request Sep 10, 2024

SIM-BE-Remove randomization of validators configuration #439

Closed

cristiam86 previously approved these changes Sep 10, 2024

View reviewed changes

AgustinRamiroDiaz force-pushed the 359-sim-refactor-how-inference-providers-are-added-and-configured branch from 4d0ba72 to 36ed535 Compare September 10, 2024 11:03

AgustinRamiroDiaz marked this pull request as draft September 10, 2024 16:32

cristiam86 deleted the branch main September 11, 2024 09:57

cristiam86 closed this Sep 11, 2024

cristiam86 reopened this Sep 11, 2024

cristiam86 changed the base branch from staging to main September 11, 2024 10:01

AgustinRamiroDiaz added the run-tests label Sep 11, 2024

AgustinRamiroDiaz linked an issue Sep 11, 2024 that may be closed by this pull request

SIM-Refactor how inference providers are added and configured #359

Closed

AgustinRamiroDiaz added 10 commits September 12, 2024 08:46

add jsf for json schema

a11b8ef

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

configure pytest vscode

db9b895

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

configure dockerignore

fd379bc

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

improve schema

fd8fc06

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

test hypothesis_jsonschema

cef6c64

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

test other libraries

1575c13

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

add default providers

66e3f4e

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

move tests

bec7836

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

refactor

def5e9e

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

improve schema

ed43cc8

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

fix tests

f778e7c

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz force-pushed the 359-sim-refactor-how-inference-providers-are-added-and-configured branch from 978c463 to f778e7c Compare September 12, 2024 11:50

AgustinRamiroDiaz added 4 commits September 12, 2024 08:56

test

7ba4058

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

test

329ec43

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

fix tests

59f8f88

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

improve schema

0461bf1

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz commented Sep 12, 2024

View reviewed changes

AgustinRamiroDiaz added 3 commits September 12, 2024 13:51

unify HeuristAI and OpenAI plugins

95f2ec9

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

fix tests

59e2885

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

start adding anthropic plugin

74d8acf

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz linked an issue Sep 13, 2024 that may be closed by this pull request

SIM-BE-Remove randomization of validators configuration #439

Closed

AgustinRamiroDiaz added 3 commits September 13, 2024 08:24

fix: register anthropic

749d8ea

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

fix validators in the frontend

048886e

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

fix endpoint

25b39c9

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz linked an issue Sep 13, 2024 that may be closed by this pull request

Add new OpenAI Models #496

Closed

AgustinRamiroDiaz added 2 commits September 13, 2024 11:28

add new openai models

770adc6

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

fix anthropic plugin

8d2600f

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz marked this pull request as ready for review September 13, 2024 15:19

AgustinRamiroDiaz requested a review from cristiam86 September 13, 2024 15:20

use protocolt instead of ABC for Plugin interface

6f85c65

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

cristiam86 approved these changes Sep 16, 2024

View reviewed changes

feat: load default providers in a thread and cache

a16920c

Signed-off-by: Agustín Ramiro Díaz <[email protected]>

AgustinRamiroDiaz requested a review from cristiam86 September 16, 2024 18:31

cristiam86 approved these changes Sep 17, 2024

View reviewed changes

AgustinRamiroDiaz merged commit c089b16 into main Sep 17, 2024
18 of 19 checks passed

github-actions bot added the released label Sep 18, 2024

cristiam86 deleted the 359-sim-refactor-how-inference-providers-are-added-and-configured branch October 16, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): refactor how inference providers are added and configured #475

feat(backend): refactor how inference providers are added and configured #475

AgustinRamiroDiaz commented Sep 5, 2024 •

edited

Loading

AgustinRamiroDiaz Sep 6, 2024

AgustinRamiroDiaz Sep 6, 2024

AgustinRamiroDiaz Sep 6, 2024

AgustinRamiroDiaz Sep 6, 2024

AgustinRamiroDiaz Sep 6, 2024

AgustinRamiroDiaz Sep 6, 2024

cristiam86 Sep 10, 2024 •

edited

Loading

AgustinRamiroDiaz Sep 10, 2024

AgustinRamiroDiaz Sep 6, 2024

cristiam86 left a comment

cristiam86 Sep 10, 2024

cristiam86 Sep 10, 2024

AgustinRamiroDiaz Sep 10, 2024

AgustinRamiroDiaz Sep 10, 2024

cristiam86 Sep 10, 2024

AgustinRamiroDiaz Sep 10, 2024

cristiam86 Sep 10, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

AgustinRamiroDiaz Sep 12, 2024

AgustinRamiroDiaz commented Sep 13, 2024

cristiam86 left a comment

AgustinRamiroDiaz commented Sep 16, 2024

github-actions bot commented Sep 18, 2024

		provider: Mapped[str] = mapped_column(String(255))
		model: Mapped[str] = mapped_column(String(255))

		@@ -0,0 +1,44 @@
		from tests.common.request import payload, post_request_localhost

feat(backend): refactor how inference providers are added and configured #475

feat(backend): refactor how inference providers are added and configured #475

Conversation

AgustinRamiroDiaz commented Sep 5, 2024 • edited Loading

What

Expected flow for new providers

Why

Testing done

Decisions made

Checks

Reviewing tips

User facing release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristiam86 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristiam86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AgustinRamiroDiaz commented Sep 13, 2024

cristiam86 left a comment

Choose a reason for hiding this comment

AgustinRamiroDiaz commented Sep 16, 2024

github-actions bot commented Sep 18, 2024

AgustinRamiroDiaz commented Sep 5, 2024 •

edited

Loading

cristiam86 Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading