-
Notifications
You must be signed in to change notification settings - Fork 685
Create pending_user_response.py #13881
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
Conversation
Python script for Workflow automation for pending_user_response.yml based on the following: - If an issue or PR has 'needs-user-input' label, and it has been 30 days since the last comment, comment and tag the user asking them to take a look. - And 30 days after this comment from the workflow, if no response, close the issue with a comment stating the user did not respond and they can re-open if needed. - If the user responds, remove the 'needs-user-input' label.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13881
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (4 Unrelated Failures)As of commit d27291b with merge base 6d8583d ( BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
Workflow automation for addressing issues/PRs pending user response. - follows script in pending_user_response.py
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.
-
The code removes the label when a user responds, but never adds it initially. There's no logic for when the need-user-input label gets applied in the first place.
-
The code could create multiple reminder comments if run multiple times, since it only checks if not auto_comments but doesn't prevent duplicate reminders. For example, you want to make explicitly check that it doesn't comment more than once in a week.
if not auto_comments: | ||
if last_comment and (now - last_comment.created_at).days >= DAYS_BEFORE_REMINDER: | ||
# Tag the issue author or PR author | ||
user = issue.user.login | ||
# issue.create_comment(REMINDER_COMMENT.format(user)) | ||
print(f"[VALIDATION] Would remind {user} on issue/PR #{issue.number}") | ||
|
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 you could improve as follow to make it more robust:
if not auto_comments:
- Find last comment from the relevant user
- Only remind if the last user comment is old enough
# Case 2: Automation comment exists, but no user response after 30 more days | ||
elif auto_comments: | ||
last_auto = auto_comments[-1] | ||
# Any user response after automation? |
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.
Check for ANY response after the last automation comment (not just from the original author)
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.
Check for ANY response after the last automation comment (not just from the original author)
This condition could still include cases where the user has not responded yet, and would remove the 'need-user-input' label unnecessarily. By adding an author-specific comment, I am checking if the user is the one who has responded after the bot reminder.
Hi @mergennachin, thanks for the review. The duplicate comment issue is a valid concern, and will make changes accordingly to avoid it. I have some queries regarding the other aspects:
I feel the 'need-user-input' label should be added manually, since the best (and only) way to recognize if a discussion involves a user to get back is by reading the reviewer's comments. Many issues I came across had discussions where the last comment, even though made by a PyTorch Edge reviewer, was a discussion within the engineering team/partner team. |
# repo = g.get_repo(REPO_NAME) | ||
|
||
print("[VALIDATION] Would connect to Github and fetch repo:", REPO_NAME) | ||
# issues = repo.get_issues(state='open', labels=[LABEL]) |
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.
Can you re-enable the this API call in order to allow testing of fetching issues? We can still leave the actual commenting and labeling disabled.
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.
Enabled these lines now.
Uncommented start of main() [lines 21-25 impacted]
On this point, my recommendation was to manually add the label, as it may be difficult to automatically infer when we are actually waiting on a user response. For example, if a maintainer mentions that something is being actively worked on, we wouldn't want to auto-label as pending user response and ping them for a comment to avoid closing. I'm open to changing the approach, though. |
now = datetime.datetime.utcnow() | ||
|
||
# Simulate issues for validation workflow | ||
issues = [] # Replace with mock issues if needed |
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.
Can you remove this line now that we're fetching the actual issues? Once that's ready and the lint job is passing, I think we can merge it to start dry-run testing in parallel with the ongoing discussion about the staling policy.
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, and added workflow dispatch. Lmk if it is good to merge
|
||
on: | ||
schedule: | ||
- cron: '0 8 * * 1' # runs every Monday at 8:00 UTC |
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.
Can you also add on workflow dispatch to allow manual triggering? Here's an example: https://github.com/pytorch/executorch/blob/main/.github/workflows/trunk.yml#L11
Added a reminder logic to comment only if no reminder in last 7 days
Python script for Workflow automation for pending_user_response.yml based on the following: - If an issue or PR has 'needs-user-input' label, and it has been 30 days since the last comment, comment and tag the user asking them to take a look. - And 30 days after this comment from the workflow, if no response, close the issue with a comment stating the user did not respond and they can re-open if needed. - If the user responds, remove the 'needs-user-input' label.
Python script for Workflow automation for pending_user_response.yml based on the following: