Fix #2643 - groupchat model registration

[Matteo-Frattaroli]

Why are these changes needed?

When using a custom model client with GroupChatManager, the underlying GroupChat internally defines two ConversableAgents that receive the llm_config passed to GroupChatManager but don't handle model registration (and there's no way to do so externally if not subclassing GroupChat and overriding the two methods that do so). This contribution allows a user to pass a ModelClient or a list of ModelClient through a 'model_client_cls' attribute used for automatic registration of custom models only.

Related issue number

Closes #2643

Checks

• I've included any doc changes needed for https://microsoft.github.io/autogen/. See https://microsoft.github.io/autogen/docs/Contribute#documentation to build and test documentation locally.
• I've added tests (if relevant) corresponding to the changes introduced in this PR.
• I've made sure all auto checks have passed.

[Matteo-Frattaroli]

@microsoft-github-policy-service agree

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 38.88889% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 39.74%. Comparing base (11d9336) to head (745d44f).
Report is 1 commits behind head on main.

Files	Patch %	Lines
autogen/agentchat/groupchat.py	38.88%	21 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2696      +/-   ##
==========================================
+ Coverage   33.60%   39.74%   +6.14%     
==========================================
  Files          87       87              
  Lines        9336     9363      +27     
  Branches     1987     2149     +162     
==========================================
+ Hits         3137     3721     +584     
+ Misses       5933     5318     -615     
- Partials      266      324      +58     

Flag	Coverage Δ
unittest	12.70% <27.77%> (?)
unittests	39.03% <38.88%> (+5.43%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[ekzhu]

Add a unit test with mocks to check if the select speaker agent has gotten the propagated custom client.

[Matteo-Frattaroli]

@ekzhu @sonichi @marklysze I completed all the changes you requested but after merging main several builds are failing because of a missing package (namely "packaging").

@ekzhu I see you submitted two commits before because of this bug on requests.

I'm pretty sure that as for the latter, this problem with the "packaging" library is not related to my code. Could you maybe take a look at your earliest convenience?

Do you think after solving this the PR could be approved?

[marklysze]

@Matteo-Frattaroli, thanks for the hard work on this! Your code has come at just the right time as I'm testing a new custom client class for Mistral and I wanted to test Group Chat :).

My first run worked with your code as is, which is great... I'll test more...

[Matteo-Frattaroli]

@marklysze thank you for the positive feedback!

As I'm sure you noticed though there is some problem with the CI which is why I rolled back the latest merge from master to try and isolate the problem locally. Once this is fixed I'll resolve the conflicts (which is really just about a method i extracted to create the agents) and maybe ask for a new review.

Meanwhile if you have any directions on the current issue I can try to help (even if I know that you're probably already looking into it and are certainly more capable of doing so than me)

[marklysze]

@marklysze thank you for the positive feedback!

As I'm sure you noticed though there is some problem with the CI which is why I rolled back the latest merge from master to try and isolate the problem locally. Once this is fixed I'll resolve the conflicts (which is really just about a method i extracted to create the agents) and maybe ask for a new review.

Meanwhile if you have any directions on the current issue I can try to help (even if I know that you're probably already looking into it and are certainly more capable of doing so than me)

Ah, okay.... do you have any output of the issues, I just see the current openai ones which I think are normal as they require approval before they can be run (I believe)...

[Matteo-Frattaroli]

@marklysze thank you for the positive feedback!
As I'm sure you noticed though there is some problem with the CI which is why I rolled back the latest merge from master to try and isolate the problem locally. Once this is fixed I'll resolve the conflicts (which is really just about a method i extracted to create the agents) and maybe ask for a new review.
Meanwhile if you have any directions on the current issue I can try to help (even if I know that you're probably already looking into it and are certainly more capable of doing so than me)

Ah, okay.... do you have any output of the issues, I just see the current openai ones which I think are normal as they require approval before they can be run (I believe)...

Hi mark, everything seems okay here because I reverted the latest merge of master which includes the problematic commit.
Check out this pipeline: as you can see the issue is related to a missing dependency (packaging)

[marklysze]

Hi mark, everything seems okay here because I reverted the latest merge of master which includes the problematic commit. Check out this pipeline: as you can see the issue is related to a missing dependency (packaging)

Ah, I see... perhaps that is a GitHub issue? Maybe one of the core team members can comment as I'm not familiar with that side of things, unfortunately...

[gitguardian]

⚠️ GitGuardian has uncovered 3 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
10493810	Triggered	Generic Password	97fa339	notebook/agentchat_pgvector_RetrieveChat.ipynb	View secret
10493810	Triggered	Generic Password	97fa339	notebook/agentchat_pgvector_RetrieveChat.ipynb	View secret
10493810	Triggered	Generic Password	97fa339	notebook/agentchat_pgvector_RetrieveChat.ipynb	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secrets safely. Learn here the best practices.
3. Revoke and rotate these secrets.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

[sonichi]

There is one approach to solve the problem in a more fundamental way: Move the register_model_client out of the agents. Instead, make it a function to manipulate configs. Then, pass the materialized configs to agents so that the agents never need to worry about register_model_client.
We can implement it in a different PR, if you wish. If we merge this PR now, some code in this PR will be removed though.

[Matteo-Frattaroli]

There is one approach to solve the problem in a more fundamental way: Move the register_model_client out of the agents. Instead, make it a function to manipulate configs. Then, pass the materialized configs to agents so that the agents never need to worry about register_model_client. We can implement it in a different PR, if you wish. If we merge this PR now, some code in this PR will be removed though.

Hi @sonichi, that's clearly a solution but it would affect several parts of the codebase and lead to a breaking change.

Since what you're proposing is a pretty massive intervention, if meanwhile you plan on merging this PR it would be nice, at least to have the possibility of using custom models with group chats while the solution you proposed is being implemented.

@ekzhu if that's the case I'd ask you to close the change request related to the unit test, as it was implemented.

[Matteo-Frattaroli]

Hello again @sonichi - I was reflecting on your idea during my lunch break and looking at the code I'm not 100% sure that it would completely resolve this problem if we want to preserve the actual functionality.

If I understood your idea correctly you're proposing of just using a config which would then handle registration automatically or at least to convert register_model_client from a method to a static independent function.

As of now though register_model_client requires some instance attributes (namely the list of _clients available for the OpenAIWrapper used by an agent) to check that the model client class that we're trying to register is indeed present in the config and create an instance of the same custom model client in the _clients attribute itself.

We would then still need to somehow get access to the agents that are created internally in the GroupChat to answer the question "for which agent am i registering this model client?". Differently the only option would be to limit the freedom of choice and register a certain model client for all the agent of a give type.
Otherwise it could be possible to reimplement the constructor of all the existing agents to handle registration given only the config (i imagine in some sort of automatic fancy, as I've done here).

Am I missing something? Please let me know if you had a different idea in mind