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

fix: ensure unique instances for each Service subclass using Map #332

Closed
wants to merge 1 commit into from

Conversation

tcm390
Copy link
Collaborator

@tcm390 tcm390 commented Nov 15, 2024

Previously, the Service base class used a single static instance, which caused
all subclasses to share the same instance. This update replaces the static
instance with a Map to manage unique instances for each subclass

@lalalune
Copy link
Member

Many of the services load up models and such and we probably only want one instance, ever, enforcing the singleton pattern, if we add a map won't we have several instances of each class? That's why I made it this service pattern.

@tcm390
Copy link
Collaborator Author

tcm390 commented Nov 15, 2024

Many of the services load up models and such and we probably only want one instance, ever, enforcing the singleton pattern, if we add a map won't we have several instances of each class? That's why I made it this service pattern.

I understand, but in the context of the Discord bot, we require two services: ISpeechService and ITranscriptionService. (Also, there are IVideoService and IBrowserService, but I'm not sure if these are necessary for the bot.) Would you prefer that I implement separate methods and instance variables for each of these services, or would you prefer a different approach? Let me know how you'd like to proceed :)

@lalalune
Copy link
Member

I want one class of each service shared across all running agents, imagine we are running 10 agents, we want them to share the transcription service

@monilpat
Copy link
Collaborator

I agree with @lalalune we should close this PR as the singleton pattern would work best in this usecase.

@jkbrooks jkbrooks closed this Nov 18, 2024
@tcm390
Copy link
Collaborator Author

tcm390 commented Nov 18, 2024

I agree with @lalalune we should close this PR as the singleton pattern would work best in this usecase.

@monilpat thanks for the review but from my understanding, the current implementation ensures that each service type has its own singleton instance. This means only one instance of each subclass is created and reused. Could you clarify if the intent is different?

@monilpat
Copy link
Collaborator

monilpat commented Nov 19, 2024

I agree with @lalalune we should close this PR as the singleton pattern would work best in this usecase.

@monilpat thanks for the review but from my understanding, the current implementation ensures that each service type has its own singleton instance. This means only one instance of each subclass is created and reused. Could you clarify if the intent is different?

Yeah of course anytime! The intent is not different. I just share @lalalune 's concern around multiple instances of service rather than a 1-> many from service -> agents. Happy to add more context, but will defer @lalalune as he has more context!

@shakkernerd shakkernerd deleted the tcm-single-instance-issue branch January 31, 2025 22:19
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.

4 participants