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

Add SageMaker as a LLM provider #1947

Merged
merged 5 commits into from
Jan 22, 2025
Merged

Conversation

bobbywlindsey
Copy link
Contributor

@bobbywlindsey bobbywlindsey commented Jan 21, 2025

Closes #1896

Changes made:

  1. Add SageMaker as a LLM provider when bootstrapping new CrewAI projects via the crewai create crew command (this is already included by choosing "Other" --> "SageMaker")
  2. Add documentation detailing how to configure SageMaker LLMs in CrewAI

@joaomdmoura
Copy link
Collaborator

Disclaimer: This review was made by a crew of AI Agents.

Code Review Comment: Integration of Amazon SageMaker as an LLM Provider

Overview

This pull request introduces Amazon SageMaker as a new LLM provider in the crewAI framework. The modifications span across two main files: docs/concepts/llms.mdx and src/crewai/cli/constants.py.

Documentation Changes (llms.mdx)

Positive Aspects:

  • The documentation adheres to existing structures, making it easier for users to follow.
  • It effectively outlines environment variable requirements, contributing to clearer instructions for setup.
  • Example usage provided is highly valuable for end-users.

Suggestions for Improvement:

  1. Clarification of Endpoint Requirements: Enhance the documentation with detailed formatting requirements for SageMaker endpoints.

    # Amazon SageMaker Models - Enterprise-grade
    # llm: sagemaker/<your-sagemaker-endpoint>
    # Format: sagemaker/endpoint-name-in-your-region
    # Example: sagemaker/jumpstart-dft-meta-textgeneration-llama-2-7b
  2. Add Prerequisites for SageMaker: Include clear information on what is needed before users can utilize SageMaker, such as an active AWS account and necessary permissions.

    <Accordion title="Amazon SageMaker">
        ### Prerequisites
        - Active AWS account with SageMaker access.
        - Deployed SageMaker endpoint.
        - IAM permissions for SageMaker invocation.

CLI Constants Changes (constants.py)

Positive Aspects:

  • Implementation is consistent with how other LLM providers are integrated.
  • Appropriate handling of AWS credentials is included, promoting best practices.

Suggestions for Improvement:

  1. Endpoint Validation: Introduce validation patterns for SageMaker endpoints to ensure correct formatting:

    PROVIDER_PATTERNS = {
        # ... existing patterns ...
        "sagemaker": r"^sagemaker/[a-zA-Z0-9-]+$",
    }
  2. Realistic Endpoint Examples: Include realistic examples of SageMaker endpoints to assist users.

    "sagemaker": [
        "sagemaker/jumpstart-dft-meta-textgeneration-llama-2-7b",
        "sagemaker/huggingface-llm-falcon-40b",
        "sagemaker/custom-llm-endpoint",
    ],
  3. User Guidance Messages: Adding clear and informative validation messages would guide users during configuration.

    PROVIDER_VALIDATION_MESSAGES = {
        # ... existing messages ...
        "sagemaker": {
            "endpoint": "SageMaker endpoint must start with 'sagemaker/' followed by the endpoint name.",
            "credentials": "AWS credentials and region are required for SageMaker access."
        }
    }

Security Considerations

  1. It is essential to include guidance on the scope of IAM permissions required to minimize risks.
  2. Recommend the use of role-based authentication instead of static access keys.
  3. Suggest implementing credential rotation best practices.

General Recommendations

  1. Error Handling: Enhance error handling to address SageMaker-specific exceptions, thus providing clear feedback to users about issues:

    try:
        response = sagemaker_runtime.invoke_endpoint(...)
    except ClientError as e:
        if e.response['Error']['Code'] == 'ValidationException':
            raise ValueError("Invalid SageMaker endpoint configuration.")
        elif e.response['Error']['Code'] == 'ModelNotReadyException':
            raise RuntimeError("SageMaker model endpoint is not ready.")
        else:
            raise
  2. SageMaker-Specific Configuration Options: Enable additional options within the LLM initialization for customization and better user control:

    llm = LLM(
        model="sagemaker/endpoint-name",
        custom_attributes="",  # SageMaker custom attributes
        timeout=60,  # Endpoint timeout in seconds
        max_retries=3  # Number of retry attempts
    )

Testing Recommendations

  1. Incorporate unit tests to validate SageMaker configurations.
  2. Include integration tests that simulate interactions with a mock SageMaker endpoint.
  3. Conduct end-to-end tests with a real SageMaker endpoint set up.
  4. Ensure credential validation tests are part of the testing suite.

Documentation TODOs

  1. Add a troubleshooting section for typical SageMaker issues users might face.
  2. Discuss cost considerations and optimization strategies within the SageMaker context.
  3. Provide resources linking to AWS SageMaker documentation for deeper insights.
  4. Include details about regional availability for different SageMaker services.

Conclusion

The PR introduces a significant enhancement to the crewAI framework through the addition of SageMaker as an LLM provider. While the implementation is solid, addressing the suggestions above will bolster the overall effectiveness and user experience related to this integration.

@bhancockio bhancockio merged commit e27a150 into crewAIInc:main Jan 22, 2025
4 checks passed
devin-ai-integration bot pushed a commit that referenced this pull request Feb 9, 2025
* Add SageMaker as a LLM provider

* Removed unnecessary constants; updated docs to align with bootstrap naming convention

---------

Co-authored-by: Brandon Hancock (bhancock_ai) <109994880+bhancockio@users.noreply.github.com>
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.

[FEATURE] Add Amazon SageMaker as a provider
3 participants