- 
                Notifications
    You must be signed in to change notification settings 
- Fork 486
documentation: Add multimcpclientsessionmanager notebook example #2168
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
documentation: Add multimcpclientsessionmanager notebook example #2168
Conversation
| Code Review: MCP Session Manager Notebook ExampleThank you for converting the Python example to a comprehensive Jupyter notebook! This is a significant improvement in documentation quality. Strengths
 Issues & Recommendations1. Critical: LLM Configuration InconsistencyThe notebook should follow AG2 conventions from notebook/contributing.md: Use config_list_from_json instead of hardcoding API keys directly. 2. Missing Installation InstructionsAdd installation requirements section before Setup and Imports, following the pattern from notebook/contributing.md for notebooks with extra dependencies (nest-asyncio). 3. Test Coverage IssueThe notebook will fail automated testing because it requires external servers and makes actual API calls. Recommendation: Add skip_test metadata explaining it requires external MCP servers. 4. Runtime Issues
 5. Model NameUsing gpt-4o without availability check. Consider gpt-4-turbo or document the requirement. Summary & Action ItemsMust Fix: 
 Should Fix: Nice to Have: Overall AssessmentThis is an excellent contribution that significantly improves documentation. The conversion is very well done with comprehensive explanations. After addressing the critical items, this will be a valuable resource. Rating: 4/5 - Great work! Just needs minor adjustments to follow AG2 conventions. Great job @priyansh4320! | 
| Code Review for PR #2168Thanks for converting the MCP Client Session Manager example from Python to a Jupyter notebook! This is a valuable improvement for documentation. I've reviewed the changes and have the following feedback: ✅ Strengths
 🔍 Issues FoundCritical Issues1. Inconsistent LLM Configuration Pattern  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @priyansh4320!
Why are these changes needed?
Updated an example for MultiMCPClientSession Manager from .py to .ipynb example
Related issue number
#2166
#2144
Checks