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

docs: Update installation instructions for clarity #288

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tadeodonegana
Copy link

@tadeodonegana tadeodonegana commented Mar 14, 2025

Update installation instructions in README to acknowledge issues #269 and #111

Used the official docs to enhance the existing example and make it more straightforward.

Demonstration of the updated installation flow:

installation-flow.mp4

Motivation and Context

Issues #269 and #111.

How Has This Been Tested?

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Copy link

@13rac1 13rac1 left a comment

Choose a reason for hiding this comment

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

This is far more clear, thank you! Note: I do not have access to approve.

Rendered View: https://github.com/modelcontextprotocol/python-sdk/blob/d9e27a89/README.md#installation

README.md Outdated
Comment on lines 79 to 81
# Create virtual environment and activate it
uv venv
source .venv/bin/activate # On Windows use: .venv\Scripts\activate

Choose a reason for hiding this comment

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

Suggested change
# Create virtual environment and activate it
uv venv
source .venv/bin/activate # On Windows use: .venv\Scripts\activate

There's no need to explicitly create and activate the virtual environment, uv takes care of that implicitly. See https://docs.astral.sh/uv/concepts/projects/layout/#the-project-environment

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your comment, @taha-yassine. I added it because the official MCP docs explicitly mention the virtual environment. It might be a good idea to review those docs as well.

I have removed the explicit creation of the virtual environment in this PR in order to proceed. Thanks!

README.md Outdated
source .venv/bin/activate # On Windows use: .venv\Scripts\activate

# Install MCP with CLI tools and the httpx package
uv add "mcp[cli]" httpx
Copy link
Member

Choose a reason for hiding this comment

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

Why would you need httpx? It's a dependency of mcp already.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your comment, @Kludex.

I originally added it because the official MCP docs explicitly mention it—likely due to the example provided in the documentation.

Since, as you mentioned, the dependency isn’t needed to run the python-sdk, I’ve removed it from this PR. Gracias!

Copy link
Member

@dsp-ant dsp-ant left a comment

Choose a reason for hiding this comment

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

I appreciate the effort. I don't think we need additional instructions to create a project. If we want to go down the path, we should skip the explicit creation of the .venv, since uv run will manage this for us, and I don't want to confuse people with additional instructions about venv management.

@tadeodonegana tadeodonegana requested a review from dsp-ant March 17, 2025 20:54
@tadeodonegana
Copy link
Author

I have applied the requested changes. @dsp-ant, I would appreciate another review of these changes to see if we can move forward. Thanks!

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.

5 participants