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

[MRG] the local web server to display the MLE suggestions #177

Merged
merged 8 commits into from
Sep 6, 2024

Conversation

huangyz0918
Copy link
Contributor

@huangyz0918 huangyz0918 commented Sep 5, 2024

User description

Closes #169

The backend server can be started under the mle project by using mle serve.
The web is placed outside the mle-agent SDK, and should be launched individually.

Before submitting this PR, please make sure you have:

  • confirmed all checks still pass OR confirm CI build passes.
  • verified that any code or assets from external sources are properly credited in comments and/or in
    the credit file.

PR Type

enhancement, dependencies


Description

  • Introduced a FastAPI server with a root endpoint and CORS middleware.
  • Added a new CLI command serve to start the FastAPI server with configurable host and port.
  • Updated requirements.txt to include fastapi and uvicorn dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
cli.py
Add CLI command to start FastAPI server                                   

mle/cli.py

  • Added import for uvicorn and app from mle.server.
  • Introduced a new CLI command serve to start the FastAPI server.
  • Configured server host and port options with default values.
  • +11/-0   
    __init__.py
    Initialize server module with app import                                 

    mle/server/init.py

    • Initialized the server module by importing app.
    +1/-0     
    app.py
    Implement FastAPI application with CORS middleware             

    mle/server/app.py

  • Created a FastAPI application instance.
  • Added CORS middleware to allow all origins and methods.
  • Defined a root endpoint returning a welcome message.
  • +22/-0   
    Dependencies
    requirements.txt
    Add FastAPI and Uvicorn to dependencies                                   

    requirements.txt

    • Added fastapi and uvicorn as dependencies.
    +2/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Sep 5, 2024
    Copy link

    github-actions bot commented Sep 5, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Security Concern
    The CORS configuration in the FastAPI application allows all origins, methods, and headers. This can expose the server to cross-origin resource sharing issues, especially if sensitive data is handled. Consider restricting the allowed origins and methods based on the actual requirements.

    Copy link

    github-actions bot commented Sep 5, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Externalize CORS settings to enhance security and maintainability

    It's recommended to externalize the CORS policy settings to a configuration file or
    environment variables to avoid hardcoding values, which can lead to security risks
    if not managed properly.

    mle/server/app.py [7-13]

    +cors_origins = os.getenv("CORS_ORIGINS", "*").split(",")
     app.add_middleware(
         CORSMiddleware,
    -    allow_origins=["*"],
    +    allow_origins=cors_origins,
         allow_credentials=True,
         allow_methods=["*"],
         allow_headers=["*"],
     )
     
    Suggestion importance[1-10]: 9

    Why: Externalizing CORS settings to environment variables or a configuration file enhances security and maintainability by avoiding hardcoded values, which can be a security risk.

    9
    Possible issue
    Add exception handling for the server start-up to improve robustness

    Consider handling exceptions for the uvicorn.run method to gracefully handle
    potential server errors such as address already in use, or other server-related
    issues. This will improve the robustness of the server command.

    mle/cli.py [134]

    -uvicorn.run(app, host=host, port=port)
    +try:
    +    uvicorn.run(app, host=host, port=port)
    +except Exception as e:
    +    click.echo(f"Failed to start server: {str(e)}")
    +    exit(1)
     
    Suggestion importance[1-10]: 8

    Why: Adding exception handling for the uvicorn.run method is a good practice to ensure that server-related issues are handled gracefully, improving the robustness of the application.

    8
    Implement exception handling in the root endpoint for better error management

    Add exception handling for the root endpoint to manage unexpected errors gracefully
    and provide a user-friendly error message.

    mle/server/app.py [16-22]

     @app.get("/")
     def root():
         """
         read_root: read the root.
         :return: the root.
         """
    -    return {"Welcome to": "MLE-Agent!"}
    +    try:
    +        return {"Welcome to": "MLE-Agent!"}
    +    except Exception as e:
    +        return {"error": str(e)}
     
    Suggestion importance[1-10]: 6

    Why: Adding exception handling to the root endpoint can help manage unexpected errors gracefully, although the likelihood of errors in this simple endpoint is low.

    6
    Enhancement
    Replace click.echo with logging for better output management

    Add logging instead of using click.echo for server start-up messages to provide more
    detailed and configurable output, which can be critical for debugging and monitoring
    in production environments.

    mle/cli.py [133]

    -click.echo(f"Starting server on {host}:{port}")
    +import logging
    +logging.info(f"Starting server on {host}:{port}")
     
    Suggestion importance[1-10]: 7

    Why: Using logging instead of click.echo provides more detailed and configurable output, which is beneficial for debugging and monitoring in production environments.

    7

    @dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 5, 2024
    @dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 5, 2024
    @huangyz0918 huangyz0918 changed the title [WIP] the local web server to display the MLE suggestions [MRG] the local web server to display the MLE suggestions Sep 5, 2024
    Copy link
    Contributor

    @HuaizhengZhang HuaizhengZhang left a comment

    Choose a reason for hiding this comment

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

    could you write down the how users use this feature?

    @dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 6, 2024
    @huangyz0918
    Copy link
    Contributor Author

    could you write down the how users use this feature?

    Yes, sure! I will update the document when this feature is well-tested

    @HuaizhengZhang
    Copy link
    Contributor

    free to merge so we can test it

    @leeeizhang
    Copy link
    Collaborator

    awesome! we can merge at first 👨🏻‍💻

    @huangyz0918 huangyz0918 merged commit 13c4b88 into main Sep 6, 2024
    3 checks passed
    @huangyz0918 huangyz0918 deleted the feat/web-server branch September 6, 2024 16:05
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    dependencies enhancement New feature or request lgtm This PR has been approved by a maintainer Review effort [1-5]: 2 size:XL This PR changes 500-999 lines, ignoring generated files.
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Web application of MLE-Agent
    3 participants