fix: pluralise checkpoints.get() #3562
Conversation
📝 Walkthrough""" WalkthroughThe changes introduce support for handling multiple checkpoint outputs in Snakemake's workflow management. A new utility function generates Cartesian products of wildcard lists, and the checkpoint logic is updated to process all combinations, accumulating jobs or missing outputs as needed. Exception handling and related methods are updated to work with collections of files instead of single files. Internal methods in rules are adjusted to handle multiple missing checkpoint files consistently. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Rule
participant Checkpoint
participant Exception
User->>Rule: expand_input / expand_params
Rule->>Checkpoint: get(**wildcards)
Checkpoint->>Checkpoint: dictproduct(wildcard_lists)
loop For each wildcard combination
Checkpoint->>Checkpoint: expand outputs
Checkpoint->>Checkpoint: check if outputs exist
alt Outputs complete
Checkpoint->>Rule: accumulate CheckpointJob
else Outputs missing
Checkpoint->>Checkpoint: collect missing outputs
end
end
alt No missing outputs
Checkpoint->>Rule: return list of CheckpointJobs
else Missing outputs exist
Checkpoint->>Exception: raise IncompleteCheckpointException(missing_outputs)
Exception-->>Rule: exception.targetfiles
end
Rule->>User: Handle jobs or missing outputs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/snakemake/exceptions.py (1)
551-569: Plural-aware attribute but still singular wording & potential backward-compat breakThe constructor now accepts
targetfilesand stores them onself.targetfiles, but
• the explanatory error text still speaks of a single “checkpoint output”; consider making it plural for clarity.
• callers that were accessing the previousexception.targetfilewill nowAttributeErrorunless all call-sites were updated (we already saw fixes inrules.py, but there may be more).A low-cost way to keep old extensions working is to provide a deprecated alias:
@@ - self.targetfiles = [checkpoint_target(targetfile) for targetfile in targetfiles] + self.targetfiles = [ + checkpoint_target(targetfile) for targetfile in targetfiles + ] + + # Backwards-compat shim (to be removed in a future major release) + if self.targetfiles: + self.targetfile = self.targetfiles[0]You may also want to s/“output is”/“outputs are”/ in the message string.
src/snakemake/checkpoints.py (1)
50-56: nit: preferdefover assignedlambda& remove assert side-effectStyle guideline (E731) – avoid assigning a
lambdato a variable.-def dictproduct(listdict): - """itertools.product() for dict of lists that yields a dict for each combination of values""" - import itertools - assert(all(isinstance(v, list) for v in listdict.values())) - for x in itertools.product(*listdict.values()): - yield dict(zip(listdict.keys(), x)) +def dictproduct(listdict): + """ + Cartesian product over a *dict of lists*. + Yields a new dict for every combination of values. + """ + import itertools + + if not all(isinstance(v, list) for v in listdict.values()): + raise ValueError("All wildcard values must be lists to build a cartesian product.") + + for combination in itertools.product(*listdict.values()): + yield dict(zip(listdict.keys(), combination))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake/checkpoints.py(2 hunks)src/snakemake/exceptions.py(2 hunks)src/snakemake/rules.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/exceptions.pysrc/snakemake/rules.pysrc/snakemake/checkpoints.py
🧬 Code Graph Analysis (2)
src/snakemake/exceptions.py (1)
src/snakemake/io/__init__.py (1)
checkpoint_target(1297-1298)
src/snakemake/rules.py (1)
src/snakemake/common/tbdstring.py (1)
TBDString(3-96)
🪛 Ruff (0.8.2)
src/snakemake/checkpoints.py
71-71: Do not assign a lambda expression, use a def
Rewrite listify as a def
(E731)
⏰ Context from checks skipped due to timeout of 90000ms (36)
- GitHub Check: tests (10, macos-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (1)
src/snakemake/rules.py (1)
867-871:expand_input: returning a list may violate downstream type assumptions
handle_incomplete_checkpoint()now returnsexception.targetfiles, i.e. a list.
_apply_wildcards()will subsequently iterate over that list and treat each entry like a regular path string, which is fine, but only if every consumer of the returned value is ready for multiple elements. Please double-check any logic that expected a single checkpoint target (e.g. duplicate-detection, ancient-flag propagation).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/snakemake/checkpoints.py (1)
81-98:⚠️ Potential issueLogic bug:
missing_outputsalways populated → checkpoint never considered completeBecause
missing_outputs.append(...)is executed unconditionally on line 95, the list is non-empty even when all outputs are present, so anIncompleteCheckpointExceptionwill always be raised.if self.checkpoints.created_output: missing_output = set(output) - set(self.checkpoints.created_output) if not missing_output: complete_jobs.append(CheckpointJob(self.rule, output)) else: logger.debug( f"Missing checkpoint output for {self.rule.name} " f"(wildcards: {wc}): {','.join(missing_output)} of {','.join(output)}" ) - missing_outputs.append(checkpoint_target(output[0])) + # only record when something is actually missing + missing_outputs.append(checkpoint_target(output[0]))This fix will ensure that only actual missing outputs are tracked, allowing the function to properly return complete jobs when all required outputs exist.
🧹 Nitpick comments (3)
src/snakemake/checkpoints.py (3)
74-80: Rewrite lambda as a proper functionInstead of assigning a lambda expression to a variable, define a proper function. This improves readability and follows Python best practices.
- listify = lambda x: ( - [ - x, - ] - if not isinstance(x, list) - else x - ) + def listify(x): + return [x] if not isinstance(x, list) else x🧰 Tools
🪛 Ruff (0.8.2)
74-80: Do not assign a
lambdaexpression, use adefRewrite
listifyas adef(E731)
81-98: Consider adding explicit condition checking for empty checkpoint outputsThe current logic doesn't handle the case where
self.checkpoints.created_outputis empty clearly. It would be better to have explicit handling for this case to make the logic more robust.for wc in dictproduct(listified): output, _ = self.rule.expand_output(wc) if self.checkpoints.created_output: missing_output = set(output) - set(self.checkpoints.created_output) if not missing_output: complete_jobs.append(CheckpointJob(self.rule, output)) else: logger.debug( f"Missing checkpoint output for {self.rule.name} " f"(wildcards: {wc}): {','.join(missing_output)} of {','.join(output)}" ) + missing_outputs.append(checkpoint_target(output[0])) + else: + # If there are no created outputs yet, all outputs are missing + logger.debug( + f"No checkpoint outputs created yet for {self.rule.name} (wildcards: {wc})" + ) + missing_outputs.append(checkpoint_target(output[0])) - missing_outputs.append(checkpoint_target(output[0]))
96-98: Add a sanity check for the case when all outputs are completeThis would be a good opportunity to add a check to immediately return complete jobs if all combinations of wildcards produce complete outputs, before attempting to handle missing outputs.
for wc in dictproduct(listified): # ... existing code ... + # If nothing is missing across *all* combinations, return immediately + if not missing_outputs: + return complete_jobs + if not missing_outputs: return complete_jobs raise IncompleteCheckpointException(self.rule, missing_outputs)This redundancy ensures correct behavior even if the logic changes in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/checkpoints.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/checkpoints.py
🧬 Code Graph Analysis (1)
src/snakemake/checkpoints.py (3)
src/snakemake/io/__init__.py (1)
checkpoint_target(1297-1298)src/snakemake/rules.py (5)
output(301-302)output(1367-1368)expand_output(993-1004)name(129-130)name(133-134)src/snakemake/exceptions.py (1)
IncompleteCheckpointException(550-568)
🪛 Ruff (0.8.2)
src/snakemake/checkpoints.py
74-80: Do not assign a lambda expression, use a def
Rewrite listify as a def
(E731)
⏰ Context from checks skipped due to timeout of 90000ms (33)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, macos-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, macos-latest, py312)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, macos-latest, py312)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (2)
src/snakemake/checkpoints.py (2)
51-58: Good implementation of dictionary Cartesian product utilityThis new utility function provides a clean implementation for generating combinations of dictionary values, which is necessary for the pluralized
checkpoints.get()function. The implementation is clear and well-documented, using Python's built-in itertools to efficiently generate all combinations.
51-98: Verify pluralization behavior with error-handling testsThe implementation for pluralizing
checkpoints.get()looks good overall, but should be thoroughly tested with various edge cases:
- Multiple wildcards with list values
- Mix of single and list values for wildcards
- Empty lists for wildcard values
- Missing outputs for some combinations but not others
Do you have or can you develop test cases that verify these scenarios? This would help ensure the implementation correctly handles all potential use cases, especially around error conditions.
🧰 Tools
🪛 Ruff (0.8.2)
74-80: Do not assign a
lambdaexpression, use adefRewrite
listifyas adef(E731)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/snakemake/checkpoints.py (1)
74-80: Replace lambda with regular function definitionUsing a lambda for function assignment is against Python best practices. Consider rewriting
listifyas a regular function.- listify = lambda x: ( - [ - x, - ] - if not isinstance(x, list) - else x - ) + def listify(x): + if not isinstance(x, list): + return [x] + return x🧰 Tools
🪛 Ruff (0.8.2)
74-80: Do not assign a
lambdaexpression, use adefRewrite
listifyas adef(E731)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/snakemake/checkpoints.py(2 hunks)src/snakemake/rules.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/snakemake/rules.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/checkpoints.py
🧬 Code Graph Analysis (1)
src/snakemake/checkpoints.py (3)
src/snakemake/io/__init__.py (1)
checkpoint_target(1297-1298)src/snakemake/rules.py (5)
output(301-302)output(1367-1368)expand_output(993-1004)name(129-130)name(133-134)src/snakemake/exceptions.py (1)
IncompleteCheckpointException(550-568)
🪛 Ruff (0.8.2)
src/snakemake/checkpoints.py
74-80: Do not assign a lambda expression, use a def
Rewrite listify as a def
(E731)
⏰ Context from checks skipped due to timeout of 90000ms (35)
- GitHub Check: tests (10, macos-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (2)
src/snakemake/checkpoints.py (2)
51-58: Implementation ofdictproductlooks good!This utility function correctly generates the Cartesian product of dictionary values, effectively enabling the checkbox behavior to match
expand()as described in the PR objectives. The implementation is clean and uses the efficientitertools.productfunction.
81-98: Logic for handling multiple wildcard combinations looks goodThis implementation properly handles the Cartesian product of wildcard values, checks for completeness of outputs, and accumulates jobs or missing outputs as needed. The fix properly addresses the issue where missing outputs were always populated - now they're only added when something is actually missing.
The overall implementation aligns well with the PR objective of making
checkpoints.get()behave similarly toexpand()by taking the Cartesian product of all wildcard values.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/snakemake/checkpoints.py (3)
74-76: Consider defininglistifyas a regular function instead of a lambdaThe helper function is correctly implemented but using a nested function definition with
defwould be more standard than a lambda assignment.- def listify(x): - return [x] if not isinstance(x, list) else x + def listify(x): + if not isinstance(x, list): + return [x] + return x
91-91: Consider adding a comment explaining the single-output tracking strategyThis line tracks only the first missing output per wildcard combination. Based on the retrieved learning, this is intentional and matches the previous behavior, but it might be helpful to add a comment clarifying this design decision for future developers.
- missing_outputs.append(checkpoint_target(output[0])) + # Track only the first output - if one output is missing, the checkpoint is considered incomplete + missing_outputs.append(checkpoint_target(output[0]))
93-93: Same tracking strategy in the "no outputs" caseAs with line 91, this also tracks only the first output. The same comment recommendation applies here for consistency.
- missing_outputs.append(checkpoint_target(output[0])) + # Track only the first output - if one output is missing, the checkpoint is considered incomplete + missing_outputs.append(checkpoint_target(output[0]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/checkpoints.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/checkpoints.py
🧠 Learnings (1)
src/snakemake/checkpoints.py (1)
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.375Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
⏰ Context from checks skipped due to timeout of 90000ms (36)
- GitHub Check: tests (10, macos-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: apidocs
- GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (2)
src/snakemake/checkpoints.py (2)
51-58: Well-implemented utility function for Cartesian product generationThis
dictproductfunction elegantly handles generating Cartesian products while maintaining the dictionary structure. It correctly implements the core functionality needed for the pluralizedcheckpoints.get()behavior described in the PR objectives.
77-96: Good implementation of pluralized checkpoint.get() with proper handling of wildcard combinationsThe implementation correctly handles multiple wildcard combinations as intended by the PR objective. It:
- Transforms all wildcard values into lists
- Processes each combination using the new
dictproductfunction- Correctly tracks complete and missing outputs
- Returns multiple complete checkpoint jobs when possible
The approach aligns with the behavior described in the retrieved learning - tracking only the first missing output for each checkpoint combination is sufficient since if one output is missing, all outputs for that combination are considered incomplete.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake/checkpoints.py (1)
51-58: Well-implemented Cartesian product utility functionThe
dictproductfunction provides a clean implementation to generate Cartesian products from dictionary values. It aligns well with the PR objective to makecheckpoints.get()handle multiple wildcard values.While the assertion ensures all values are lists, consider if other iterable types (tuples, sets) might be valid inputs in the future:
- assert all(isinstance(v, list) for v in listdict.values()) + assert all(hasattr(v, "__iter__") and not isinstance(v, str) for v in listdict.values())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake/checkpoints.py(2 hunks)src/snakemake/exceptions.py(2 hunks)src/snakemake/rules.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/snakemake/exceptions.py
- src/snakemake/rules.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/checkpoints.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
src/snakemake/checkpoints.py (1)
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
⏰ Context from checks skipped due to timeout of 90000ms (35)
- GitHub Check: tests (10, macos-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (1)
src/snakemake/checkpoints.py (1)
74-96:Details
❓ Verification inconclusive
Good implementation of pluralized
get()method for checkpointsThe modified
get()method now handles wildcards with list values by taking the Cartesian product of all combinations, similar to Snakemake'sexpand()function. This successfully addresses the issue of having to call.get()multiple times.I notice you're maintaining the behavior of only tracking the first missing output per combination at line 91 and 93, which is consistent with the existing Snakemake behavior as mentioned in your feedback on a previous review.
🏁 Script executed:
#!/bin/bash # Look for existing callers of checkpoints.get() to ensure they can handle multiple jobs rg -A 3 "checkpoints\.[a-zA-Z_]+\.get\(" --glob "*.py" --glob "!src/snakemake/checkpoints.py"Length of output: 95
I didn’t find any matches with the initial pattern, so let’s broaden the search to catch all potential calls to
checkpoints.<name>.get():
🏁 Script executed:
#!/bin/bash # Refined search for any checkpoints.<NAME>.get() usage across the repo rg -n "checkpoints\.[[:alnum:]_]*\.get\(" --glob "*.py"Length of output: 57
Please verify that all
checkpoints.<rule>.get()calls handle multiple jobsI didn’t find any internal usages of
checkpoints.<name>.get()in the codebase—please manually confirm that any existing callers (including user-defined checkpoint invocations in Snakefiles or other workflow code) correctly process a list ofCheckpointJobinstances instead of a single job.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/snakemake/checkpoints.py (3)
51-58: dictproduct unnecessarily restricted tolist– relax to any iterable & drop runtime assertThe helper breaks if a caller passes a tuple,
set, orrange, even thoughitertools.product()can handle any iterable.
By enforcinglist, you create avoidable friction for users and clutter the call-site withlist()casts that bring no semantic benefit.-def dictproduct(listdict): - """itertools.product() for dict of lists that yields a dict for each combination of values""" - import itertools - - assert all(isinstance(v, list) for v in listdict.values()) - for x in itertools.product(*listdict.values()): - yield dict(zip(listdict.keys(), x)) +def dictproduct(mapping): + """Cartesian product of an arbitrary iterable-valued mapping. + + Yields one dict per combination, preserving key order. + """ + import itertools + + for combo in itertools.product(*mapping.values()): + yield dict(zip(mapping.keys(), combo))This change removes the overly tight contract while keeping behaviour identical for current callers.
74-78: Locallistify()duplicates utility logic – consider extracting once
listify()is re-defined every call toget(). Moving it to a private module-level helper (or reusing an existing utility if one exists in the codebase) avoids the micro-overhead and improves discoverability.
Not critical, but worth tidying while you are touching this area.
80-96: Duplicate entries possible inmissing_outputs– de-duplicate before raisingWhen several wildcard combinations miss the same first output file, that path is appended repeatedly.
This can bloat theIncompleteCheckpointExceptionmessage without adding value.- missing_outputs = [] + missing_outputs = set() ... - missing_outputs.append(checkpoint_target(output[0])) + missing_outputs.add(checkpoint_target(output[0])) ... - missing_outputs.append(checkpoint_target(output[0])) + missing_outputs.add(checkpoint_target(output[0])) ... - if not missing_outputs: + if not missing_outputs: return complete_jobs - raise IncompleteCheckpointException(self.rule, missing_outputs) + raise IncompleteCheckpointException(self.rule, sorted(missing_outputs))Switching to a set keeps the user-facing diagnostics concise while preserving existing semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake/checkpoints.py(2 hunks)src/snakemake/exceptions.py(2 hunks)src/snakemake/rules.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/snakemake/exceptions.py
- src/snakemake/rules.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/checkpoints.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
src/snakemake/checkpoints.py (1)
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
🪛 Pylint (3.3.7)
src/snakemake/checkpoints.py
[refactor] 99-99: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (35)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (6, macos-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: apidocs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/snakemake/checkpoints.py (3)
51-58: Consider broadeningdictproductinput contract and move import to top-levelLimiting values strictly to
listprevents passing tuples, sets or other iterables that behave the same. In practice wildcard collections are often created withtuples. Dropping the hardassert(or relaxing it tocollections.abc.Iterable) would make the helper more generally useful and avoid surprisingAssertionErrors.-assert all(isinstance(v, list) for v in listdict.values()) -for x in itertools.product(*listdict.values()): +from collections.abc import Iterable +assert all(isinstance(v, Iterable) and not isinstance(v, str) for v in listdict.values()) +for x in itertools.product(*(list(v) for v in listdict.values())): yield dict(zip(listdict.keys(), x))This keeps behaviour identical for lists while accepting any non-string iterable.
74-78: Minor: locallistify()could be inlined
listifyis a one-liner used once to normalise wildcard values. Inlining it saves an indirection and avoids nesting an extra function insideget()(ruff(E731) complaints are gone already, but the helper is still arguably overkill).- def listify(x): - return [x] if not isinstance(x, list) else x - - listified = {k: listify(v) for k, v in wildcards.items()} + listified = {k: v if isinstance(v, list) else [v] for k, v in wildcards.items()}
101-108: ExposewildcardsinCheckpointJob.__slots__but not in__repr__/equality
wildcardsis now stored butCheckpointJobstill lacks a customised__repr__,__eq__, etc.
This makes debugging harder and can lead to subtle bugs if two jobs with identical output but
different wildcard combinations compare equal.Consider implementing
__repr__(and, if relied upon,__hash__/__eq__) that includes
wildcardsfor clarity.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 101-101: Too few public methods (0/2)
(R0903)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/checkpoints.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/checkpoints.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
src/snakemake/checkpoints.py (1)
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
🧬 Code Graph Analysis (1)
src/snakemake/checkpoints.py (3)
src/snakemake/io/__init__.py (1)
checkpoint_target(1319-1320)src/snakemake/rules.py (5)
output(301-302)output(1383-1384)expand_output(993-1004)name(129-130)name(133-134)src/snakemake/exceptions.py (1)
IncompleteCheckpointException(550-568)
🪛 Pylint (3.3.7)
src/snakemake/checkpoints.py
[refactor] 101-101: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (35)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (8, macos-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, macos-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: apidocs
🔇 Additional comments (1)
src/snakemake/checkpoints.py (1)
95-98:Details
✅ Verification successful
Return-type change (
Checkpoint.get()may now returnlist[CheckpointJob]) – audit callersCallers that previously assumed a single
CheckpointJob(e.g. accessed.outputdirectly) will now
receive a list when multiple wildcard combinations are complete. This silent API broadening can
causeTypeError: 'list' object has no attribute 'output'.Please grep for
.get(usages and ensure they handle both cases (or always a list). Example script:
🏁 Script executed:
#!/bin/bash # Find potential call sites expecting a scalar result. rg --python -n "\.get\([^)]*\)" | rg -v "src/snakemake/checkpoints.py"Length of output: 481
🏁 Script executed:
#!/bin/bash # Re-run grep for .get( calls in Python files, excluding the checkpoints implementation. rg -n "\.get\(" -g "*.py" | rg -v "src/snakemake/checkpoints.py"Length of output: 9239
No Checkpoint.get() usages detected – no audit required
I searched for calls to
Checkpoint.get()outside of its own implementation and found none in the codebase. There are no external callers that would now receive a list instead of a singleCheckpointJob.
(fixed version after coderabbit halucination)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/snakemake/checkpoints.py (3)
51-58: UseWorkflowErrorand avoid per-call imports
dictproduct()currently:
- Raises a bare
AssertionErrorwhen values aren’t lists – this leaks an internal detail to users.- Imports
itertoolsevery time the function is called, which is unnecessary overhead.- import itertools - assert all(isinstance(v, list) for v in listdict.values()) - for x in itertools.product(*listdict.values()): + from itertools import product + + if not all(isinstance(v, list) for v in listdict.values()): + raise WorkflowError( + "All wildcard values passed to checkpoints.get() must be lists." + ) + + for x in product(*listdict.values()): yield dict(zip(listdict.keys(), x))This surfaces a clearer, Snakemake-specific error and keeps the hot path import-free.
95-98: Document mixed return type
Checkpoint.get()can now return either a singleCheckpointJobor a list of them.
Adding a short note to the method docstring will save users from having to inspect the implementation.
102-108: Prefer canonicalWildcardsover rawdictDownstream internals typically expect a
snakemake.io.Wildcardsinstance. Storing a plaindictmay force later conversions or cause subtle mismatches.+from snakemake.io import Wildcards # add near other imports ... - self.wildcards = wildcards + # Store as canonical Wildcards for consistency + self.wildcards = Wildcards(wildcards or {})This keeps checkpoint jobs consistent with the rest of the Snakemake API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/checkpoints.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/checkpoints.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
src/snakemake/checkpoints.py (1)
Learnt from: kdm9
PR: snakemake/snakemake#3562
File: src/snakemake/checkpoints.py:90-90
Timestamp: 2025-05-06T01:37:23.382Z
Learning: In Snakemake checkpoints implementation, tracking only the first missing output for each checkpoint is sufficient, because if one output is missing, all outputs for that checkpoint are considered incomplete. This was the behavior before PR #3562 and maintained in the pluralized `checkpoints.get()` implementation.
🪛 Pylint (3.3.7)
src/snakemake/checkpoints.py
[refactor] 101-101: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (35)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, macos-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, macos-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (6, macos-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py312)
🔇 Additional comments (1)
src/snakemake/checkpoints.py (1)
80-93: Double-wrapping of checkpoint targets resolvedReplacing
checkpoint_target(output[0])withoutput[0]eliminates the earlier double-wrapping bug and restores the original semantics (only the constructor wraps). Behaviour now matches the long-standing implementation – looks good.
|
The problem is that I think this would only solve the issue with calling mutliple instances of the same checkpoint rule. However, I would rather want to solve this for multiple checkpoint rules as well, ideally with one unified syntax. I have no good strategy yet how to do that. |
This PR makes
checkpoints.get()behave similarly toexpand(), taking the Cartesian product of all wildcard values. This gets around issues with having to call.get()multiple times, like #3475. There is a bit more detail in a comment I left in that issueI have tested this locally on a workflow and it appears to behave as expected, but I would appreciate any feedback on the logic here from core devs, because I am only 90% sure I understand the way incomplete checkpoints are handled.
best,
Kevin
Summary by CodeRabbit