Skip to content

Conversation

@priyansh4320
Copy link
Collaborator

@priyansh4320 priyansh4320 commented Oct 27, 2025

Why are these changes needed?

Updated an example for MultiMCPClientSession Manager from .py to .ipynb example

Related issue number

#2166
#2144

Checks

@priyansh4320 priyansh4320 self-assigned this Oct 27, 2025
@priyansh4320 priyansh4320 linked an issue Oct 27, 2025 that may be closed by this pull request
@joggrbot
Copy link

joggrbot bot commented Oct 27, 2025

📝 Documentation Analysis

All docs are up to date! 🎉


✅ Latest commit analyzed: 7e83acb | Powered by Joggr

@claude
Copy link

claude bot commented Oct 27, 2025

Code Review: MCP Session Manager Notebook Example

Thank you for converting the Python example to a comprehensive Jupyter notebook! This is a significant improvement in documentation quality.


Strengths

  1. Excellent Documentation Structure - Clear, well-organized markdown cells with comprehensive introduction to MCPClientSessionManager
  2. Code Quality - Clean code with proper async context managers and good error handling
  3. Educational Value - Progressive complexity with excellent explanations and workflow diagrams
  4. Metadata Configuration - Proper front_matter with appropriate tags

Issues & Recommendations

1. Critical: LLM Configuration Inconsistency

The notebook should follow AG2 conventions from notebook/contributing.md:

Use config_list_from_json instead of hardcoding API keys directly.

2. Missing Installation Instructions

Add installation requirements section before Setup and Imports, following the pattern from notebook/contributing.md for notebooks with extra dependencies (nest-asyncio).

3. Test Coverage Issue

The 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

  • Wikipedia SSE server must be running at http://127.0.0.1:8000/sse
  • File paths assume notebook runs from repository root
  • Consider using Path for better portability

5. Model Name

Using gpt-4o without availability check. Consider gpt-4-turbo or document the requirement.


Summary & Action Items

Must Fix:

  1. Change LLM config to use config_list_from_json pattern
  2. Add skip_test metadata
  3. Add installation requirements section

Should Fix:
4. Add prerequisites section listing external dependencies
5. Make file paths more portable

Nice to Have:
6. Add expected output examples
7. Add troubleshooting section


Overall Assessment

This 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!

@priyansh4320 priyansh4320 changed the title 2166 documentation add multimcpclientsessionmanager notebook example documentation: Add multimcpclientsessionmanager notebook example Oct 27, 2025
@claude
Copy link

claude bot commented Oct 28, 2025

Code Review for PR #2168

Thanks 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

  1. Excellent Documentation: The markdown cells provide comprehensive explanations of concepts, with clear sections on "What is MCPClientSessionManager?", "How It Works", and "Key Takeaways".

  2. Good Structure: The notebook follows a logical flow from setup → configuration → implementation → execution → explanation.

  3. Proper Metadata: The front_matter metadata is correctly configured with appropriate tags and description.

  4. Educational Value: The progressive disclosure of concepts (starting with basics, then showing implementation) is pedagogically sound.


🔍 Issues Found

Critical Issues

1. Inconsistent LLM Configuration Pattern ⚠️

Location: Cell #6 (notebook/agentchat_mcp_session_manager_example.ipynb:106)

The notebook uses:

llm_config = LLMConfig(
    config_list=[{
        "model": "gpt-4o",
        "api_type": "openai",
        "api_key": OPENAI_API_KEY,
    }],
    temperature=0.7,
)

According to notebook/contributing.md, the recommended pattern for consistency is:

import autogen

config_list = autogen.config_list_from_json(
    env_or_file="OAI_CONFIG_LIST",
)

Recommendation:

  • Either update to use config_list_from_json for consistency with other notebooks
  • OR add a markdown cell after the LLM config explaining the configuration (as per contributing.md guidelines)

2. Missing Installation Instructions ⚠️

The notebook lacks installation instructions. Per notebook/contributing.md, notebooks should include an info box with installation requirements.

Recommendation: Add after the introduction:

````{=mdx}
:::info Requirements
Some extra dependencies are needed for this notebook, which can be installed via pip:

```bash
pip install autogen[openai] mcp

For more information, please refer to the installation guide.
:::

```

---

### **Code Quality Issues**

#### 3. **Deprecated Import Path**
**Location**: Cell #4 (notebook/agentchat_mcp_session_manager_example.ipynb:78)

The original .py file used:
```python
from autogen import LLMConfig
```

But the notebook should import from the full path for clarity:
```python
from autogen import ConversableAgent, LLMConfig
```

This is correct in the notebook - good job! ✓

#### 4. **Potential Runtime Issue: SSE Server Not Running**
**Location**: Cell #16 and #18

The notebook tries to connect to `http://127.0.0.1:8000/sse` but there's no guarantee the server is running. While there IS a note in cell #15, this could still cause confusion.

**Recommendation**: 
- Add a `skip_test` metadata field to prevent CI failures
- Consider adding a try-except wrapper in the example code with a helpful error message

#### 5. **Code Duplication in Agent System Messages**
**Location**: Cell #12 and Cell #14

The `RESEARCH_AGENT_PROMPT` and `TOOL_PROMPT` both list the same server information:
```
1. ArxivServer: to search for papers on arXiv
2. WikipediaServer: to search for articles on Wikipedia
```

**Recommendation**: Consider generating this dynamically from `mcp_config` to maintain DRY principles and make it easier to add new servers.

---

### **Documentation Issues**

#### 6. **Incorrect Comparison in "Key Takeaways"**
**Location**: Cell #19 (final cell)

The comparison shows:
```python
**Traditional (keep session open):**
async with stdio_client(...) as (read, write):
    async with ClientSession(read, write) as session:
        # Do all work here
```

However, looking at `notebook/agentchat_mcp_arxiv.ipynb`, the actual traditional pattern imports from `mcp.client.stdio`, not from autogen:
```python
from mcp import ClientSession, StdioServerParameters
from mcp.client.stdio import stdio_client
```

**Recommendation**: Make sure the comparison accurately reflects what users would actually write, including proper imports.

#### 7. **Missing Context on When NOT to Use This Pattern**
While the notebook explains when to use `MCPClientSessionManager`, it doesn't explain when the traditional approach might be better (e.g., if you only have one server and want to reuse the connection).

---

### **Minor Issues**

#### 8. **Author Information**
The notebook lists author as "Priyanshu Deshmukh" but the original .py file had no author comment. Verify this is correct (it likely is based on PR author).

#### 9. **Grok Notebook Metadata Fix** ✓
Good catch on fixing the description in `agentchat_grok_example.ipynb`! This was clearly wrong.

#### 10. **Hardcoded Model in Example**
The notebook uses `gpt-4o` which may not be available to all users. Consider using `gpt-4o-mini` or making it clear this is configurable.

---

## 🔒 **Security Considerations**

- ✅ API keys are loaded from environment variables (good practice)
- ✅ No hardcoded credentials
- ⚠️ The `/tmp/arxiv_papers` path could be a security issue on shared systems, but this is acceptable for an example

---

## 🧪 **Testing Recommendations**

1. Add `skip_test` metadata since the notebook requires external servers:
```json
{
  "metadata": {
    "skip_test": "Requires external MCP servers (ArXiv stdio and Wikipedia SSE) to be running"
  }
}
```

2. Consider creating a mock/test mode that doesn't require real servers for CI testing.

---

## 📝 **Suggestions for Improvement**

1. **Add error handling examples**: Show what happens when a server is unreachable
2. **Add server health check**: Demonstrate checking if servers are available before querying
3. **Show server list retrieval**: Demonstrate how to query `mcp_config` for available servers
4. **Add timing information**: Show the performance difference between keeping sessions open vs. on-demand

---

## Summary

**Overall Assessment**: ✅ This is a solid contribution that significantly improves the documentation by providing an interactive notebook example.

**Must Fix Before Merge**:
1. Add installation instructions per contributing.md guidelines
2. Add `skip_test` metadata to prevent CI failures
3. Update LLM configuration to follow repository conventions

**Nice to Have**:
1. Dynamic server list generation
2. Error handling examples
3. Clarify when NOT to use this pattern

Great work on the comprehensive documentation and clear explanations! The notebook will be very helpful for users learning about MCP session management. 🎉

Copy link
Collaborator

@marklysze marklysze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @priyansh4320!

@marklysze marklysze added this pull request to the merge queue Oct 28, 2025
Merged via the queue into main with commit 0ef1d3d Oct 28, 2025
20 checks passed
@marklysze marklysze deleted the 2166-documentation-add-multimcpclientsessionmanager-notebook-example branch October 28, 2025 01:15
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.

Documentation: Add MultiMCPClientSessionManager notebook example

3 participants