refactor: group all provider/model related logic into `llm` directory #254

jrmi · 2024-11-12T20:20:06Z

This is a work in progress version of the refactoring I suggested. It's not well tested yet and there are probably a few regressions here and there but I wanted to show you the idea before going further.

I've introduced the concept of ModelManager. There is one implementation per API and they deal with the small differences we can have between model. The manager is instantiated with the model so it's available everywhere.

The idea is that everything that is specific to a model or a provider should be in the provider directory.

Let me know what you think.

Important

Refactor codebase by moving provider/model logic to providers directory and introducing ModelManager for model-specific operations.

Refactoring:
- Move provider and model-related logic to providers directory.
- Introduce ModelManager for handling model-specific logic, instantiated with the model.
Imports:
- Update imports in chat.py, commands.py, init.py, message.py, reduce.py, server/api.py, tools/chats.py, tools/youtube.py, and test_server.py to reflect new directory structure.
Functionality:
- Add supports_file and supports_streaming properties to ModelManager.
- Implement prepare_file method in AnthropicModelManager and OpenaiModelManager for handling file preparation.
Miscellaneous:
- Update logic in llm.py, llm_anthropic.py, and llm_openai.py to use ModelManager for model-specific operations.
- Adjust get_model() usage across various files to utilize the new ModelManager structure.

^{This description was created by}^{for 12a9067. It will automatically update as commits are pushed.}

ellipsis-dev

❌ Changes requested. Reviewed everything up to da39f7f in 1 minute and 18 seconds

More details

Looked at 761 lines of code in 15 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. gptme/providers/llm_openai.py:40

Draft comment:
supports_streaming is defined as a method but used as a property in chat.py. Consider changing it to a property or updating its usage.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative and refers to a file not included in the diff. It does not provide strong evidence of an issue within the changes made in this diff. The comment is not actionable based on the information provided.
I might be missing the context of how supports_streaming is used elsewhere, but the comment should focus on the changes in this diff. Without seeing chat.py, it's hard to verify the comment's validity.
The comment should be removed because it does not pertain to the changes in this diff and lacks strong evidence of an issue.
Remove the comment as it is speculative and not directly related to the changes in the diff.

Workflow ID: wflow_2xJEjSzRQfTUnaWF

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

gptme/providers/llm_anthropic.py

ErikBjare · 2024-11-12T21:51:53Z

gptme/providers/manager.py

+
+
+@dataclass()
+class ModelManager:


Why not just use ModelMeta?

I wanted to change less code but happy to implement it use ModelMeta directly, what do you think of it now?

gptme/chat.py

ErikBjare · 2024-11-12T21:53:38Z

gptme/message.py

+            file_content = get_model().manager.prepare_file(media_type, data)
+            if file_content:
+                content.append(file_content)


This is not valid, you cannot rely on get_model() returning the correct model for a given call to to_dict or _content_files_list.

To be fair, I knew you would notice that 😄. As I said in PR description it's more like a POC for now, but yeah I noticed that there are a few places where the logic is a bit different. Could we consider to return a " fake model" in these situations? May be you could give me more details to help me understand why we can't always use get_model() so that I can come up with a solution for that as well? I think it a good idea to have something consistent throughout the code.

Left some comments about this on the rest of the PR, we can make it work!

gptme/message.py

ErikBjare

Overall, I don't think these changes make much sense. You seem to have underestimated things, as you cannot just use get_model everywhere.

ellipsis-dev

❌ Changes requested. Reviewed everything up to 12a9067 in 1 minute and 7 seconds

More details

Looked at 626 lines of code in 16 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. gptme/providers/llm_openai.py:35

Draft comment:
Consider moving the _prep_o1 logic into the OpenaiModelManager class to encapsulate model-specific message preparation logic.
Reason this comment was not posted:
Confidence changes required: 50%
The refactoring introduces a new ModelManager concept and moves model/provider-specific logic into the providers directory. This is a good step towards modularizing the code. However, there are some issues with the streaming logic in llm_openai.py. The _prep_o1 function is used to prepare messages for OpenAI O1 models, which do not support the system role. This logic should be encapsulated within the ModelManager for OpenAI models.

Workflow ID: wflow_eHUd39kJaQYewjKF

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2024-11-13T08:27:25Z

gptme/chat.py

@@ -56,8 +56,12 @@ def chat(
    # init
    init(model, interactive, tool_allowlist)

-    if model and model.startswith("openai/o1") and stream:
-        logger.info("Disabled streaming for OpenAI's O1 (not supported)")
+    if get_model().manager.supports_streaming and stream:


The condition here seems incorrect. It should check if streaming is NOT supported to disable it.

Suggested change

if get_model().manager.supports_streaming and stream:

if not get_model().manager.supports_streaming and stream:

Good catch. fixed.

jrmi

Overall, I don't think these changes make much sense. You seem to have underestimated things, as you cannot just use get_model everywhere.

The idea behind my changes is to establish a clear separation between provider/model-specific code and the rest. This would help contributors easily identify where they can add provider/model-specific code and where they shouldn't. I'm confident we can find a solution that aligns both with your vision and the changes I've proposed. Wouldn't it be beneficial to remove all these scattered specific codes and improve encapsulation?

I may have underestimated some aspects, given that I don't know the codebase and all its branches as well as you do. However, I'm eager to work on this because I truly believe it would enhance the readability and maintainability of the code in the long term.

gptme/message.py

jrmi · 2024-11-13T21:47:53Z

gptme/chat.py

@@ -56,8 +56,12 @@ def chat(
    # init
    init(model, interactive, tool_allowlist)

-    if model and model.startswith("openai/o1") and stream:
-        logger.info("Disabled streaming for OpenAI's O1 (not supported)")
+    if get_model().manager.supports_streaming and stream:


Good catch. fixed.

gptme/chat.py

gptme/providers/llm_anthropic.py

jrmi · 2024-11-13T21:50:14Z

gptme/providers/manager.py

+
+
+@dataclass()
+class ModelManager:


I wanted to change less code but happy to implement it use ModelMeta directly, what do you think of it now?

jrmi · 2024-11-13T21:55:55Z

gptme/message.py

+            file_content = get_model().manager.prepare_file(media_type, data)
+            if file_content:
+                content.append(file_content)


To be fair, I knew you would notice that 😄. As I said in PR description it's more like a POC for now, but yeah I noticed that there are a few places where the logic is a bit different. Could we consider to return a " fake model" in these situations? May be you could give me more details to help me understand why we can't always use get_model() so that I can come up with a solution for that as well? I think it a good idea to have something consistent throughout the code.

gptme/message.py

ErikBjare

Alright, I like that a lot better!

A few ideas on how we can get this merged, it will really be an improvement and I appreciate your efforts.

Sorry if I was grumpy before, I've been sick this week.

gptme/llm/__init__.py

gptme/llm/llm_anthropic.py

gptme/message.py

gptme/chat.py

ErikBjare · 2024-11-14T12:52:29Z

gptme/message.py

+            file_content = get_model().manager.prepare_file(media_type, data)
+            if file_content:
+                content.append(file_content)


Left some comments about this on the rest of the PR, we can make it work!

jrmi · 2024-11-14T20:54:26Z

Hey @ErikBjare I think we are going in the right direction 🚀 . I applied your suggestions and it's much better and it makes a lot of sense. I hope I understood everything. My only doubt is about the code duplication in the file handling in each providers. We could do something, but I'm not sure it's necessary/a good idea. Let me know what you think.

Same as before, it's not polished, i think one test is failing but I'll fix that when I'm sure what I've done is ok with you.

A few ideas on how we can get this merged, it will really be an improvement and I appreciate your efforts.
Sorry if I was grumpy before, I've been sick this week.

Sure, no problem. Happy to help.

- Add 'unknown' as valid provider type in ModelMeta - Handle missing 'files' key in Anthropic message dict - Organize imports and improve type hints

- Fix: Remove 'files' field from message dict in anthropic provider - Refactor: Extract file processing logic in both providers - Split handle_files into smaller functions - Make internal functions private with underscore prefix - Improve code organization and readability

ErikBjare · 2024-11-22T11:49:08Z

Rebased and force pushed.

Typechecks and tests passing locally 🎉

ErikBjare · 2024-11-22T11:56:32Z

Going to go ahead and merge this, I'll fix any remaining issues in follow-up PRs.

Thanks a lot for pulling through with this @jrmi! 💪

You dropped this: 👑

jrmi marked this pull request as draft November 12, 2024 20:20

ellipsis-dev bot reviewed Nov 12, 2024

View reviewed changes

gptme/providers/llm_anthropic.py Outdated Show resolved Hide resolved

gptme/providers/llm_anthropic.py Outdated Show resolved Hide resolved

jrmi force-pushed the refactor-providers branch from da39f7f to 13db6a5 Compare November 12, 2024 20:23

jrmi changed the title ~~Grouping all provider/model related logic into provider directory~~ Grouping all provider/model related logic into providers directory Nov 12, 2024

jrmi force-pushed the refactor-providers branch 3 times, most recently from 0c1ffbc to 12a9067 Compare November 12, 2024 20:37

ErikBjare reviewed Nov 12, 2024

View reviewed changes

gptme/chat.py Outdated Show resolved Hide resolved

ErikBjare reviewed Nov 12, 2024

View reviewed changes

gptme/message.py Show resolved Hide resolved

ErikBjare reviewed Nov 12, 2024

View reviewed changes

gptme/message.py Show resolved Hide resolved

ErikBjare reviewed Nov 12, 2024

View reviewed changes

0xbrayo marked this pull request as ready for review November 13, 2024 08:26

ellipsis-dev bot reviewed Nov 13, 2024

View reviewed changes

jrmi commented Nov 13, 2024

View reviewed changes

jrmi requested a review from ErikBjare November 13, 2024 22:36

jrmi force-pushed the refactor-providers branch 4 times, most recently from 9c4298b to 7770a71 Compare November 13, 2024 22:51

ErikBjare reviewed Nov 14, 2024

View reviewed changes

jrmi force-pushed the refactor-providers branch from 7770a71 to 1f4b338 Compare November 14, 2024 20:41

jrmi force-pushed the refactor-providers branch 2 times, most recently from 5d85bff to 22be8eb Compare November 14, 2024 21:30

jrmi requested a review from ErikBjare November 14, 2024 21:32

jrmi changed the title ~~Grouping all provider/model related logic into providers directory~~ Grouping all provider/model related logic into llm directory Nov 16, 2024

jrmi added 2 commits November 22, 2024 12:43

Move provider files in provider directory

22a3852

Move all specific code into abstraction

e5ac888

jrmi and others added 4 commits November 22, 2024 12:48

Fix first round of feedback

f934561

Second round of feedbacks

cd964a9

fix(llm): improve type hints and handle missing files

99868b2

- Add 'unknown' as valid provider type in ModelMeta - Handle missing 'files' key in Anthropic message dict - Organize imports and improve type hints

ErikBjare force-pushed the refactor-providers branch from 9d2f1fe to b4bc091 Compare November 22, 2024 11:48

ErikBjare changed the title ~~Grouping all provider/model related logic into llm directory~~ refactor: group all provider/model related logic into llm directory Nov 22, 2024

ErikBjare approved these changes Nov 22, 2024

View reviewed changes

ErikBjare merged commit ac3c173 into ErikBjare:master Nov 22, 2024
4 of 7 checks passed

ErikBjare mentioned this pull request Nov 22, 2024

fix: model provider handling and vision support metadata #269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: group all provider/model related logic into `llm` directory #254

refactor: group all provider/model related logic into `llm` directory #254

jrmi commented Nov 12, 2024 •

edited by ellipsis-dev bot

Loading

ellipsis-dev bot left a comment

ErikBjare Nov 12, 2024

jrmi Nov 13, 2024

ErikBjare Nov 12, 2024 •

edited

Loading

jrmi Nov 13, 2024

ErikBjare Nov 14, 2024

ErikBjare left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot Nov 13, 2024

jrmi Nov 13, 2024

jrmi left a comment

jrmi Nov 13, 2024

jrmi Nov 13, 2024

jrmi Nov 13, 2024

ErikBjare left a comment

ErikBjare Nov 14, 2024

jrmi commented Nov 14, 2024 •

edited

Loading

ErikBjare commented Nov 22, 2024

ErikBjare commented Nov 22, 2024

	if get_model().manager.supports_streaming and stream:
	if not get_model().manager.supports_streaming and stream:



		@dataclass()
		class ModelManager:



		@dataclass()
		class ModelManager:

refactor: group all provider/model related logic into llm directory #254

refactor: group all provider/model related logic into llm directory #254

Conversation

jrmi commented Nov 12, 2024 • edited by ellipsis-dev bot Loading

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikBjare Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikBjare left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrmi 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

ErikBjare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrmi commented Nov 14, 2024 • edited Loading

ErikBjare commented Nov 22, 2024

ErikBjare commented Nov 22, 2024

refactor: group all provider/model related logic into `llm` directory #254

refactor: group all provider/model related logic into `llm` directory #254

jrmi commented Nov 12, 2024 •

edited by ellipsis-dev bot

Loading

ErikBjare Nov 12, 2024 •

edited

Loading

jrmi commented Nov 14, 2024 •

edited

Loading