fix: add files to PR and switch reretry with tenacity#119
Open
fgvieira wants to merge 9 commits intosnakemake:mainfrom
Open
fix: add files to PR and switch reretry with tenacity#119fgvieira wants to merge 9 commits intosnakemake:mainfrom
fgvieira wants to merge 9 commits intosnakemake:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates the retry mechanism from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakedeploy/prs.py`:
- Line 5: The current import and decorator use reretry-style parameters (tries,
delay) which tenacity doesn't support; update the import to bring in tenacity's
helpers (e.g., from tenacity import retry, stop_after_attempt, wait_fixed) and
replace the decorator usage "@retry(tries=..., delay=...)" with tenacity
equivalents, e.g. "@retry(stop=stop_after_attempt(<tries>),
wait=wait_fixed(<delay_seconds>))" so the retry behavior maps correctly; apply
this change where the `@retry` decorator is used (around line 59) and adjust
numeric values as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8af5ad0-1b26-404f-b662-a90d515dc3ad
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (1)
snakedeploy/prs.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakedeploy/prs.py`:
- Line 5: The import statement in prs.py has a syntax error: "from tenacity
import retry,, stop_after_attempt, wait_exponential" contains a double comma;
open the file and correct the import to list the symbols properly (ensure you
import retry, stop_after_attempt, and wait_exponential from tenacity without the
extra comma) so the module can be parsed and imported successfully.
- Around line 85-87: The except clause currently references
GithubException.UnknownObjectException (invalid attribute); update the handler
in snakedeploy/prs.py so it catches the correct exception: either import
UnknownObjectException and use "except UnknownObjectException as e" (add the
proper import at the top) or change the clause to catch the parent
"GithubException" like the earlier handler; make the rest of the logic (if
e.status != 404: raise e) remain unchanged.
fgvieira
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snakedeploy/snakemake_wrappers.py (1)
189-202:⚠️ Potential issue | 🔴 Critical
pr.create()is called multiple times whenper_snakefile_prs=False.The
pr.create()call at line 198 executes on every loop iteration regardless of theper_snakefile_prsflag. Whenper_snakefile_prs=False, this causes multiplecreate()calls on the same PR object—once per snakefile in the loop, plus once more at line 202.Previously this was masked because
self.fileswas empty (per context snippet fromsnakedeploy/prs.py:61-63), causingcreate()to return early. Now thatpr.add_file()populates files, the bug becomes active and will likely cause GitHub API errors or unexpected behavior.Proposed fix
if create_prs: assert pr is not None pr.add_file( snakefile, snakefile_content, is_updated=True, msg=f"perf: update {snakefile}.", ) - pr.create() + if per_snakefile_prs: + pr.create() if create_prs and not per_snakefile_prs: assert pr is not None pr.create()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakedeploy/snakemake_wrappers.py` around lines 189 - 202, The loop currently calls pr.create() on every iteration and again after the loop when create_prs is true, causing duplicate creates when per_snakefile_prs is False; update the code so the per-iteration pr.create() (the call immediately after pr.add_file in the loop) is only executed when per_snakefile_prs is True (i.e., guard or move that call into the per_snakefile_prs branch) and leave the single post-loop pr.create() for the per_snakefile_prs == False case; reference pr.add_file, pr.create, create_prs, and per_snakefile_prs to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@snakedeploy/snakemake_wrappers.py`:
- Around line 189-202: The loop currently calls pr.create() on every iteration
and again after the loop when create_prs is true, causing duplicate creates when
per_snakefile_prs is False; update the code so the per-iteration pr.create()
(the call immediately after pr.add_file in the loop) is only executed when
per_snakefile_prs is True (i.e., guard or move that call into the
per_snakefile_prs branch) and leave the single post-loop pr.create() for the
per_snakefile_prs == False case; reference pr.add_file, pr.create, create_prs,
and per_snakefile_prs to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d1c50b8-2eb7-43f5-bb9c-ec6749ecbd32
📒 Files selected for processing (3)
snakedeploy/prs.pysnakedeploy/snakemake_wrappers.pysnakedeploy/templates/plugins/scheduler-plugins/init.py
✅ Files skipped from review due to trivial changes (1)
- snakedeploy/templates/plugins/scheduler-plugins/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- snakedeploy/prs.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores