-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: preliminary imap_mail integration closes issue #3202 #3213
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
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
first draft still needs some tweaks! |
Wendong-Fan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @waleedalzarooni , left some initial comments
camel/toolkits/imap_mail_toolkit.py
Outdated
| # Validate required parameters | ||
| if not self.imap_server: | ||
| raise ValueError( | ||
| "IMAP server must be provided or set via IMAP_SERVER" | ||
| "environment variable" | ||
| ) | ||
| if not self.smtp_server: | ||
| raise ValueError( | ||
| "SMTP server must be provided or set via SMTP_SERVER" | ||
| "environment variable" | ||
| ) | ||
| if not self.username: | ||
| raise ValueError( | ||
| "Username must be provided or set via EMAIL_USERNAME" | ||
| "environment variable" | ||
| ) | ||
| if not self.password: | ||
| raise ValueError( | ||
| "Password must be provided or set via EMAIL_PASSWORD" | ||
| "environment variable" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use the @api_keys_required decorator make the code more tidy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems not updated?
| def _get_smtp_connection(self) -> smtplib.SMTP: | ||
| r"""Establish SMTP connection. | ||
|
|
||
| Returns: | ||
| smtplib.SMTP: Connected SMTP client | ||
| """ | ||
| if not self.smtp_server or not self.username or not self.password: | ||
| raise ValueError( | ||
| "SMTP server, username, and password must be provided" | ||
| ) | ||
|
|
||
| try: | ||
| smtp = smtplib.SMTP(self.smtp_server, self.smtp_port) | ||
| smtp.starttls() | ||
| smtp.login(self.username, self.password) | ||
| logger.info( | ||
| "Successfully connected to SMTP server %s", self.smtp_server | ||
| ) | ||
| return smtp | ||
| except Exception as e: | ||
| logger.error("Failed to connect to SMTP server: %s", e) | ||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be better to do the connect in __init__? otherwise we would need to connect eachtime the function is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I only make the connection in __init__ then there will be an issue when sequential function calls occur. After each function call the connection must be closed, unless the connection is opened at each function call this is not possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Wendong-Fan Would you prefer that I integrated the connections into the __init__ method then implemented a separate close_connections for the agent to call to close all connections at the end of an interaction (more risky as it may be missed by the agent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can implement automatic connection management with timeouts for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the timeouts preferable @Wendong-Fan ?
camel/toolkits/imap_mail_toolkit.py
Outdated
|
|
||
| def fetch_emails( | ||
| self, | ||
| folder: str = "INBOX", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the folder, how many types do we support? would be better to use Literal to list all supported type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern with using Literal here is that since we support many different email providers with slightly different naming conventions for folders, e.g. Gmail uses Sent and Outlook uses Sent Items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Literal would be better compared with string, it would be more hard for llm to fill in free string with slight difference correctly
camel/toolkits/imap_mail_toolkit.py
Outdated
| except imaplib.IMAP4.error: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not good practice to handle errors silently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, issue is fixed
| imap_server="imap.gmail.com", | ||
| smtp_server="smtp.gmail.com", | ||
| username="[email protected]", | ||
| password="your_app_password", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job @waleedalzarooni. Sensitive email credentials are hardcoded in the example, which poses security risks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, will be removed
| try: | ||
| imap = self._get_imap_connection() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of logging in inside each function, we could handle the login once in the constructor (init). This way, the connection is established when the class is initialized, and functions can directly use it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned to Wendong there is a slight concern with this approach, if I wanted to open connections in the innit method I would need to create a new close method to close all connections, I will discuss in the meeting today whether I should do this as it would add an extra method the agent would need to call, which it could forget
a7m-1st
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now that is all from me @waleedalzarooni. The example is working all right. The changes I noticed are up for discussion.
Thanks !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @waleedalzarooni , left some comments below, also added one enhance PR to update part of the content discussed below here: #3236
camel/toolkits/imap_mail_toolkit.py
Outdated
| # Validate required parameters | ||
| if not self.imap_server: | ||
| raise ValueError( | ||
| "IMAP server must be provided or set via IMAP_SERVER" | ||
| "environment variable" | ||
| ) | ||
| if not self.smtp_server: | ||
| raise ValueError( | ||
| "SMTP server must be provided or set via SMTP_SERVER" | ||
| "environment variable" | ||
| ) | ||
| if not self.username: | ||
| raise ValueError( | ||
| "Username must be provided or set via EMAIL_USERNAME" | ||
| "environment variable" | ||
| ) | ||
| if not self.password: | ||
| raise ValueError( | ||
| "Password must be provided or set via EMAIL_PASSWORD" | ||
| "environment variable" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems not updated?
camel/toolkits/imap_mail_toolkit.py
Outdated
| ) | ||
| continue | ||
|
|
||
| email_dict = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems in the fetch_emails method when email_body is neither bytes nor str, the code executes continue. However, outside of this branch, the email_message variable is used, could lead to NameError?
camel/toolkits/imap_mail_toolkit.py
Outdated
| content_type = part.get_content_type() | ||
| content_disposition = str(part.get("Content-Disposition")) | ||
|
|
||
| # Skip attachments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated comment
| def _get_smtp_connection(self) -> smtplib.SMTP: | ||
| r"""Establish SMTP connection. | ||
|
|
||
| Returns: | ||
| smtplib.SMTP: Connected SMTP client | ||
| """ | ||
| if not self.smtp_server or not self.username or not self.password: | ||
| raise ValueError( | ||
| "SMTP server, username, and password must be provided" | ||
| ) | ||
|
|
||
| try: | ||
| smtp = smtplib.SMTP(self.smtp_server, self.smtp_port) | ||
| smtp.starttls() | ||
| smtp.login(self.username, self.password) | ||
| logger.info( | ||
| "Successfully connected to SMTP server %s", self.smtp_server | ||
| ) | ||
| return smtp | ||
| except Exception as e: | ||
| logger.error("Failed to connect to SMTP server: %s", e) | ||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can implement automatic connection management with timeouts for this
camel/toolkits/imap_mail_toolkit.py
Outdated
|
|
||
| def fetch_emails( | ||
| self, | ||
| folder: str = "INBOX", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Literal would be better compared with string, it would be more hard for llm to fill in free string with slight difference correctly
|
My apologies, I haven't been active on this PR because I thought the enhance PR merged into main. Shall we go ahead with shipping this now. |
a7m-1st
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there @waleedalzarooni I have updated some test cases, tried to refine the types and added documentation. No issues from myside in runtime, apologies for the time taken.
cc: @Wendong-Fan
Description
IMAP integration into toolkit
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!