src/snakemake/workflow.py (1)

1953-1954: Good caching implementation for rule source code.

These lines add the source code caching mechanism by storing the rule's source code in rule.run_func_src when a rule function is defined. This will help prevent unnecessary reruns when rule source hasn't changed meaningfully.

Since you mentioned needing help with testing, I'd suggest a test case that:

1. Creates a workflow with a Python rule
2. Runs it once and verifies completion
3. Runs again without changes to verify it doesn't rerun (up-to-date)
4. Makes a comment-only change to the rule source
5. Verifies it still doesn't rerun
6. Makes a functional change to the source
7. Verifies it does rerun (rule marked as outdated)

Would you like me to help develop this test case for you?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/snakemake/workflow.py (2)

1717-1735: Implementation for extracting rule function source code looks solid

This new method retrieves the source code of a rule function by using the dis module to analyze bytecode and map line numbers to the original source file. The approach is well documented and handles the edge case of skipping the first line (offset 0) that corresponds to the rule definition line.

I have a minor concern about error handling - the code assumes that sourcefile is always present in self.linemaps and there's no try/except block to handle cases where line numbers might not be present in the map or issues with reading source files.

Consider adding error handling to gracefully handle cases where:

1. sourcefile isn't in self.linemaps
2. Line numbers from dis.findlinestarts() aren't in linemap
3. linecache.getline() fails to read the file

def get_rule_source(self, func):
    # This can't use `inspect` because the functions are compiled into intermediate python code
    # in parser.py and that intermediate source is not available anymore (or desirable).
    # Instead, we're using dis to retrieve the line numbers of the function in the intermediate
    # code and then map it to the original file using `self.linemaps`.
    sourcefile = func.__code__.co_filename
    line_numbers = []
-    linemap = self.linemaps[sourcefile]
+    try:
+        linemap = self.linemaps.get(sourcefile, {})
+        if not linemap:
+            return ""
+        
+        for func_offset, line in dis.findlinestarts(func.__code__):
+            # The first instruction in the compiled function is RESUME, which
+            # with snakemake is mapped to the 'rule: ' line and is not considered
+            # part of the rule source.
+            if func_offset == 0:
+                continue
+            if line in linemap:
+                line_numbers.append(linemap[line])
+        
+        return "".join(
+            [linecache.getline(sourcefile, lineno) for lineno in sorted(line_numbers)]
+        )
+    except Exception as e:
+        logger.debug(f"Could not extract source for rule function: {e}")
+        return ""
-    for func_offset, line in dis.findlinestarts(func.__code__):
-        # The first instruction in the compiled function is RESUME, which
-        # with snakemake is mapped to the 'rule: ' line and is not considered
-        # part of the rule source.
-        if func_offset == 0:
-            continue
-        if line in linemap:
-            line_numbers.append(linemap[line])
-    return "".join(
-        [linecache.getline(sourcefile, lineno) for lineno in sorted(line_numbers)]
-    )

---

1-2411: Consider adding unit tests for the source code extraction functionality

The PR objectives mention a need for assistance with developing a test for these changes. Implementing a test would be beneficial to verify that the source code extraction works correctly and that it prevents unnecessary reruns of Python rules.

I can help develop a test case that would:

1. Create a rule with a Python function
2. Extract and verify the source code
3. Test that changes to comments or formatting don't trigger reruns
4. Test that actual code changes do trigger reruns

Would you like me to draft a test implementation for this feature?

🧰 Tools

🪛 Ruff (0.8.2)

68-68: snakemake.exceptions.update_lineno imported but unused

Remove unused import: snakemake.exceptions.update_lineno

(F401)

---

75-75: snakemake.io.protected imported but unused

Remove unused import

(F401)

---

76-76: snakemake.io.temp imported but unused

Remove unused import

(F401)

---

77-77: snakemake.io.temporary imported but unused

Remove unused import

(F401)

---

78-78: snakemake.io.ancient imported but unused

Remove unused import

(F401)

---

79-79: snakemake.io.directory imported but unused

Remove unused import

(F401)

---

81-81: snakemake.io.glob_wildcards imported but unused

Remove unused import

(F401)

---

82-82: snakemake.io.flag imported but unused

Remove unused import

(F401)

---

83-83: snakemake.io.touch imported but unused

Remove unused import

(F401)

---

84-84: snakemake.io.unpack imported but unused

Remove unused import

(F401)

---

85-85: snakemake.io.local imported but unused

Remove unused import

(F401)

---

86-86: snakemake.io.pipe imported but unused

Remove unused import

(F401)

---

87-87: snakemake.io.service imported but unused

Remove unused import

(F401)

---

88-88: snakemake.io.repeat imported but unused

Remove unused import

(F401)

---

89-89: snakemake.io.report imported but unused

Remove unused import

(F401)

---

90-90: snakemake.io.multiext imported but unused

Remove unused import

(F401)

---

91-91: snakemake.io.ensure imported but unused

Remove unused import

(F401)

---

99-99: snakemake.script.script imported but unused

Remove unused import: snakemake.script.script

(F401)

---

100-100: snakemake.notebook.notebook imported but unused

Remove unused import: snakemake.notebook.notebook

(F401)

---

101-101: snakemake.wrapper.wrapper imported but unused

Remove unused import: snakemake.wrapper.wrapper

(F401)

---

102-102: snakemake.cwl.cwl imported but unused

Remove unused import: snakemake.cwl.cwl

(F401)

---

103-103: snakemake.template_rendering.render_template imported but unused

Remove unused import: snakemake.template_rendering.render_template

(F401)

---

104-104: snakemake_interface_common.utils.not_iterable imported but unused

Remove unused import: snakemake_interface_common.utils.not_iterable

(F401)

---

112-112: snakemake.common.Rules imported but unused

Remove unused import: snakemake.common.Rules

(F401)

---

132-132: snakemake.api imported but unused

Remove unused import: snakemake.api

(F401)

---

355-356: Use a single if statement instead of nested if statements

(SIM102)

---

397-397: Redefinition of unused touch from line 83

(F811)

---

750-754: Do not assign a lambda expression, use a def

Rewrite relpath as a def

(E731)

---

908-912: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

1038-1038: Redefinition of unused cwl from line 102

(F811)

---

1682-1682: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

1691-1691: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

1702-1702: Redefinition of unused report from line 89

(F811)

---

1857-1859: Multiple isinstance calls for r, merge into a single call

Merge isinstance calls for r

(SIM101)

---

1885-1886: Use a single if statement instead of nested if statements

(SIM102)

---

1945-1945: Avoid inequality comparisons to False; use if ruleinfo.container_img: for truth checks

Replace with ruleinfo.container_img

(E712)

---

2114-2116: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

2135-2138: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

2239-2239: Redefinition of unused script from line 99

(F811)

---

2239-2239: Redefinition of unused script from line 99

(F811)

---

2246-2246: Redefinition of unused notebook from line 100

(F811)

---

2246-2246: Redefinition of unused notebook from line 100

(F811)

---

2253-2253: Redefinition of unused wrapper from line 101

(F811)

---

2253-2253: Redefinition of unused wrapper from line 101

(F811)

---

2267-2267: Redefinition of unused cwl from line 102

(F811)

---

2267-2267: Redefinition of unused cwl from line 102

(F811)

---

2335-2339: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

2354-2358: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

---

2361-2365: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

tests/tests.py (1)

1907-1935: Excellent test implementation for the caching issue.

This test follows the recommended approach perfectly:

1. It runs the workflow initially and captures the outputs
2. It clears the linecache to ensure new source code is picked up
3. It replaces the Snakefile with a modified version
4. It runs the workflow again without forcing to test if the change is detected naturally
5. It runs a third time with forceall=True to get fresh outputs
6. The assertions verify the expected behavior

The test is well-structured, but consider using context managers for file operations:

- ts_a1, ts_b1 = [open(os.path.join(tmpdir, f)).read() for f in ['file_a.txt', 'file_b.txt']]
+ ts_a1, ts_b1 = [open(os.path.join(tmpdir, f)).read() and f.close() for f in ['file_a.txt', 'file_b.txt']]

Or better:

with open(os.path.join(tmpdir, 'file_a.txt')) as f_a, open(os.path.join(tmpdir, 'file_b.txt')) as f_b:
    ts_a1, ts_b1 = f_a.read(), f_b.read()

And similarly for the other file reading operations.

🧰 Tools

🪛 Ruff (0.8.2)

1912-1912: Use a context manager for opening files

(SIM115)

---

1920-1920: Use a context manager for opening files

(SIM115)

---

1923-1923: Use a context manager for opening files

(SIM115)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

tests/tests.py (1)

1907-1951: LGTM! Good test implementation for the Python rules caching mechanism.

This test effectively validates that the caching solution works correctly by:

1. Running a workflow and capturing output
2. Clearing the linecache (crucial for the fix being tested)
3. Changing the Snakefile and verifying only rules with changed code are rerun
4. Verifying that forceall works as expected

Minor improvement opportunity: consider using context managers for file handling operations.

-        ts_a1, ts_b1 = [open(os.path.join(tmpdir, f)).read() for f in output_files]
+        ts_a1, ts_b1 = [open(os.path.join(tmpdir, f)).read() for f in output_files]

-        ts_a2, ts_b2 = [open(os.path.join(tmpdir, f)).read() for f in output_files]
+        with open(os.path.join(tmpdir, f)) as file_a, open(os.path.join(tmpdir, f)) as file_b:
+            ts_a2 = file_a.read()
+            ts_b2 = file_b.read()

-        ts_a3, ts_b3 = [open(os.path.join(tmpdir, f)).read() for f in output_files]
+        ts_a3, ts_b3 = [open(os.path.join(tmpdir, f)).read() for f in output_files]

Actually, a cleaner solution would be to create a helper function:

def read_output_files(directory, files):
    results = []
    for f in files:
        with open(os.path.join(directory, f)) as file:
            results.append(file.read())
    return results

# Then use it like:
ts_a1, ts_b1 = read_output_files(tmpdir, output_files)

🧰 Tools

🪛 Ruff (0.8.2)

1914-1914: Use a context manager for opening files

(SIM115)

---

1930-1930: Use a context manager for opening files

(SIM115)

---

1939-1939: Use a context manager for opening files

(SIM115)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro