Skip to content
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.

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

refactor: consider data types for logically-grouped variables #10

Open
inspiralpatterns opened this issue Jul 12, 2023 · 3 comments
Open

Comments

@inspiralpatterns
Copy link
Collaborator

return text_result, call_data, model_config

I would always consider whether to make my code more informative and cleaner by e.g. making a specific data type out of a tuple.

Consider e.g.

from dataclasses import dataclass

@dataclass
class OpenAIResult:
    generated_text: str
    call_data: CallData
    model_config: ModelConfig

where CallData and ModelConfig are other data types as well: everything becomes much more self-documented. Furthermore, you don't have to build maps (i.e. dict) on the fly inside the methods.

You could also define a common interface, e.g. BaseLLMResult and have OpenAIResult extend it.

@stoyan-stoyanov
Copy link
Owner

I am trying to avoid defining these until I start adding LLMs from other providers. Any idea on how we can make this generic enough beforehand?

@inspiralpatterns
Copy link
Collaborator Author

inspiralpatterns commented Jul 13, 2023

I am trying to avoid defining these until I start adding LLMs from other providers. Any idea on how we can make this generic enough beforehand?

Yes: you can define abtract data types and extend them whenever you get into a different provider e.g., OpenAIResult could just extend BaseLLMResult - if we assume that you will get a result most times. 😀

The beauty of having abstract interfaces is that you do not have to think about all the possible providers at once.

@stoyan-stoyanov stoyan-stoyanov self-assigned this Jul 16, 2023
@stoyan-stoyanov stoyan-stoyanov removed their assignment Jul 28, 2023
@stoyan-stoyanov
Copy link
Owner

Reevaluating this since I added support for another LLM and the need for this becomes more obvious. My worry is that dataclasses are not serializable by default

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

No branches or pull requests

2 participants