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

thunder: Create a new package for glue #631

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aditya-nambiar
Copy link
Contributor

@aditya-nambiar aditya-nambiar commented Jan 28, 2025

Important

Add conditional import for pycurl in client.py and modify pyproject.toml for building fennel-glue-ai.

  • Imports:
    • Conditional import of pycurl in client.py to handle ImportError.
  • Build Configuration:
    • Modify pyproject.toml to include comments for building fennel-glue-ai.
    • Suggest commenting out pycurl dependency when building fennel-glue-ai.

This description was created by Ellipsis for 10945df. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to d8185e8 in 16 seconds

More details
  • Looked at 52 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. fennel/client/client.py:13
  • Draft comment:
    The import of pycurl is wrapped in a try-except block, but the code does not handle the case where pycurl is not available. Ensure that pycurl is checked for None before use to avoid runtime errors.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_7zA05MJMeoCfSbJD


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 10945df in 15 seconds

More details
  • Looked at 58 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. fennel/client/client.py:16
  • Draft comment:
    Consider adding a check for pycurl before using it in the code to avoid runtime errors if it's not installed.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import of pycurl is wrapped in a try-except block to handle the ImportError, which is a good practice for optional dependencies. However, the code does not handle the case where pycurl is not installed, which could lead to runtime errors if any functionality relying on pycurl is used without checking its availability.
2. pyproject.toml:27
  • Draft comment:
    Consider automating the process of excluding pycurl when building for fennel-glue-ai to avoid manual errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The comment in the pyproject.toml file suggests commenting out the pycurl dependency when building for fennel-glue-ai. This is a manual step that could be error-prone. It would be better to automate this process or provide a script to handle it.

Workflow ID: wflow_qaaIiXPmja9X6Txg


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

2 participants