Files that changed from the base of the PR and between 7e6e0f8 and 29598a1.

• snakemake/dag.py (1 hunks)
• snakemake/jobs.py (2 hunks)
• snakemake/persistence.py (5 hunks)

snakemake/jobs.py (4)

1648-1649: LGTM: Import of NO_PARAMS_CHANGE

The import of NO_PARAMS_CHANGE from snakemake.persistence is a good addition. It suggests that the parameter change tracking has been refined to provide more detailed information.

---

1655-1655: Improved parameter change tracking

The initialization of self.params_changed with NO_PARAMS_CHANGE instead of a boolean value indicates a more nuanced approach to tracking parameter changes. This change allows for more detailed reporting of parameter modifications.

---

1783-1785: Enhanced parameter change reporting

The __str__ method now includes the actual value of self.params_changed in the output string. This provides more detailed information about the nature of parameter changes, improving the clarity of the reason for job re-execution.

---

Line range hint 1648-1785: Overall assessment: Improved parameter change handling

The changes in the Reason class enhance Snakemake's ability to track and report parameter changes more precisely. This improvement allows for more transparent reporting of detected changes, aligning well with the pull request's objectives. The modifications are well-implemented and do not introduce any apparent issues or inconsistencies.

snakemake/persistence.py (4)

7-7: Importing dataclass is necessary

The addition of dataclass from the dataclasses module is appropriate for defining the ParamsChange class later in the code.

---

20-20: Added typing imports for type annotations

The inclusion of Any, Optional, and Set from the typing module enhances type annotations and code readability.

---

26-26: Importing TBDString for parameter handling

The import of TBDString is necessary for handling unrepresentable parameters in the _serialize_param_builtin method.

---

515-535: Confirm the comparison with TBDString

In the _serialize_param_builtin method, you check param is not TBDString to exclude TBDString parameters. Ensure that this comparison correctly excludes all instances of TBDString. If TBDString can have multiple instances, consider using not isinstance(param, TBDString) instead.

snakemake/dag.py (1)

1307-1307: Direct assignment to reason.params_changed is appropriate

The direct assignment of reason.params_changed improves code clarity and is appropriate if self.workflow.persistence.params_changed(job) returns a boolean value.

Files that changed from the base of the PR and between 29598a1 and 4f5907b.

• snakemake/persistence.py (5 hunks)

snakemake/persistence.py (4)

515-535: LGTM: Improved parameter serialization logic

The addition of TBDString to the condition in _serialize_param_builtin method ensures that it's treated as an unrepresentable parameter. This change aligns well with the new parameter serialization logic and improves the robustness of the method.

---

725-725: LGTM: Useful constant for no parameter changes

The addition of the NO_PARAMS_CHANGE constant provides a convenient and clear way to represent the absence of parameter changes. It's effectively used in the params_changed method as an initial value.

---

7-7: LGTM: Necessary imports added

The new imports (dataclass, field, Any, Optional, Set, and TBDString) have been correctly added to support the new ParamsChange dataclass, type hinting, and the updated _serialize_param_builtin method. These additions enhance the code's type safety and functionality.

Also applies to: 20-20, 26-26

---

Line range hint 1-725: Overall: Excellent enhancements to parameter change tracking and persistence

The changes in this file significantly improve the parameter change tracking and serialization logic in the Snakemake persistence module. The introduction of the ParamsChange dataclass and the refactoring of the params_changed method provide a more structured and robust approach to handling parameter changes across job executions.

Key improvements:

1. Enhanced parameter serialization with TBDString handling
2. Refactored params_changed method for better clarity and functionality
3. Well-designed ParamsChange dataclass for encapsulating parameter changes
4. Addition of the NO_PARAMS_CHANGE constant for convenience

These changes align well with the PR objective of enhancing the detection of parameter changes and ensuring more transparent reporting.

To ensure that the changes are consistent across the codebase, please run the following script to check for any other occurrences of params_changed that might need updating:

This will help verify that the new ParamsChange return value is handled correctly throughout the codebase.

✅ Verification successful

---

The occurrences of params_changed in snakemake/dag.py are consistent with the recent changes in persistence.py. Both instances correctly utilize the updated method signature:

1. self.workflow.persistence.params_changed(job) – Uses the required job parameter.
2. elif self.workflow.persistence.params_changed(job, file=f): – Appropriately provides both job and the optional file parameter.

These usages align with the enhancements made to the parameter change tracking and serialization logic, ensuring robust and clear handling of parameter changes across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of params_changed that might need updating

# Search for params_changed method calls
rg "params_changed\(" --type python -g '!persistence.py'

Length of output: 89

---

Script:

#!/bin/bash
# Description: Check for other occurrences of params_changed that might need updating

# Search for params_changed method calls in Python files excluding persistence.py
rg "params_changed\(" --glob "*.py" --glob "!snakemake/persistence.py"

Length of output: 268

---

Script:

#!/bin/bash
# Description: Extract surrounding context for params_changed calls in snakemake/dag.py

# Extract lines around the params_changed calls for better context
rg "params_changed\(" --context 3 snakemake/dag.py

Length of output: 894

Files that changed from the base of the PR and between 4f5907b and 6298c9d.

• snakemake/dag.py (1 hunks)
• snakemake/jobs.py (3 hunks)
• snakemake/persistence.py (11 hunks)
• snakemake/workflow.py (2 hunks)

• snakemake/jobs.py

snakemake/persistence.py (11)

7-7: Imports Added Correctly

The imports for dataclass, field, and the typing annotations (Any, Optional, Set) are appropriately added to support the new code changes.

Also applies to: 20-20

---

26-26: Import TBDString for Parameter Serialization

Importing TBDString ensures that parameters of this type are properly handled during serialization, preventing potential issues with unrepresentable objects.

---

36-36: Define RECORD_FORMAT_VERSION Constant

Defining the RECORD_FORMAT_VERSION as 3 establishes a clear versioning system for metadata records, facilitating future compatibility checks and migrations.

---

329-329: Include record_format_version in Metadata

Adding record_format_version to the metadata ensures that each record carries version information, which is essential for detecting outdated metadata and maintaining data integrity.

---

400-405: Add has_outdated_metadata Method

The has_outdated_metadata method correctly checks if any output files have metadata with a version older than the current RECORD_FORMAT_VERSION. This function enhances the system's ability to detect and handle outdated records.

---

427-429: Add record_format_version Method

The record_format_version method provides a convenient way to retrieve the metadata's format version for a given path, improving code readability and maintainability.

---

455-465: Refactor params_changed Method to Use ParamsChange

The params_changed method now utilizes the ParamsChange dataclass to accumulate parameter differences across output files. This refactoring enhances the clarity and robustness of parameter change detection.

---

477-480: Adjust _code_changed Method for Version Compatibility

By checking the record_format_version, the _code_changed method appropriately handles cases where reliable code comparisons are not possible due to version differences. This prevents false positives in code change detection.

---

508-512: Modify _code Method to Consider Only Shell Commands

Focusing on shell commands in the _code method is a sensible approach, as it avoids issues with pickling Python code across different versions. This change enhances the reliability of code tracking.

---

533-553: Update _serialize_param_builtin to Handle TBDString

By treating TBDString as unrepresentable, the _serialize_param_builtin method prevents potential serialization issues, ensuring that incomplete or placeholder strings do not cause errors during parameter serialization.

---

699-735: Implement ParamsChange Dataclass

The new ParamsChange dataclass is well-designed, with appropriate default initialization of only_old and only_new as empty sets. The methods __bool__, __or__, and __str__ are correctly implemented, providing useful functionality for parameter change detection and representation.

snakemake/dag.py (2)

1306-1307: Ensure has_outdated_metadata accurately detects outdated jobs

The new condition if self.workflow.persistence.has_outdated_metadata(job): introduces a check for outdated metadata. Confirm that the has_outdated_metadata method correctly identifies when a job's metadata is outdated to prevent unnecessary job executions or missing necessary updates.

---

Line range hint 1309-1317: Ensure comprehensive handling of all rerun triggers

Make sure that all specified rerun triggers (PARAMS, INPUT, CODE, SOFTWARE_ENV) are thoroughly checked and that reason is updated accordingly for each. This guarantees that any changes necessitating a job rerun are appropriately captured.

Files that changed from the base of the PR and between 6298c9d and 644d7d5.

• snakemake/workflow.py (2 hunks)

Files that changed from the base of the PR and between 644d7d5 and ac491d0.

• snakemake/persistence.py (11 hunks)
• snakemake/workflow.py (4 hunks)

snakemake/persistence.py (3)

7-7: LGTM: Improved imports and versioning

The new imports enhance type hinting, and the introduction of RECORD_FORMAT_VERSION suggests a good practice for managing data structure changes.

Also applies to: 20-20, 26-26, 31-31, 36-36

---

329-329: LGTM: Added record format versioning

Adding the record_format_version to the record is a good practice for maintaining backward compatibility and facilitating future data migrations.

---

427-429: LGTM: Added record format version getter

The record_format_version method provides a clean and convenient way to access the record format version for a specific path. This addition enhances the API for version-related operations.

snakemake/workflow.py (4)

1304-1317: Well-structured and efficient implementation.

The log_metadata_info method is a well-implemented helper function for logging metadata information about jobs. It uses efficient list comprehension for filtering jobs and modern f-strings for formatting the log message. The function is also generic, allowing it to handle different types of metadata attributes.

---

1319-1320: Good use of abstraction.

The log_missing_metadata_info method effectively uses the more generic log_metadata_info to handle the specific case of missing metadata. This approach follows the DRY principle and improves code maintainability.

---

Line range hint 1-1350: Summary: Valuable enhancements to metadata and provenance logging

The changes to snakemake/workflow.py introduce new logging capabilities for metadata and provenance information. These enhancements provide users with more detailed insights into job status and execution triggers. The implementation is generally well-structured and follows good coding practices.

Key points:

1. New helper methods for logging metadata information are concise and reusable.
2. The log_provenance_info method, while informative, could benefit from some refactoring for improved readability and maintainability.
3. Integration of new logging calls in the execute method is appropriate, but performance impact should be considered for large workflows.

Overall, these changes should improve the user experience by providing more comprehensive information about the workflow execution process.

---

1247-1248: Improved logging with additional metadata and provenance information.

The new method calls self.log_missing_metadata_info(), self.log_outdated_metadata_info(), and self.log_provenance_info() enhance the logging capabilities of the workflow execution. They provide valuable information about the metadata status of jobs and provenance information.

The placement of these calls within the execute method is appropriate:

1. Logging missing and outdated metadata when there's nothing to be done.
2. Logging provenance information in the dryrun case.

Consider the performance impact of these new logging calls, especially for large workflows. It might be beneficial to add some basic performance metrics. You can use the following script to check the execution time of these new logging methods:

This will help ensure that the new logging doesn't introduce significant overhead, especially for large workflows.

Also applies to: 1263-1263

Files that changed from the base of the PR and between ac491d0 and d92f440.

• tests/test_params_outdated_code/.snakemake/log/2024-10-14T123852.529917.snakemake.log is excluded by !**/*.log
• tests/test_params_outdated_code/expected-results/somedir/test.out is excluded by !**/*.out
• tests/test_params_outdated_code/somedir/test.out is excluded by !**/*.out

• .github/workflows/main.yml (2 hunks)
• tests/test_params_outdated_code/.snakemake/metadata/c29tZWRpci90ZXN0Lm91dA== (1 hunks)
• tests/test_params_outdated_code/Snakefile (1 hunks)
• tests/tests.py (2 hunks)

tests/test_params_outdated_code/Snakefile (2)

1-5: LGTM: Clear docstring and appropriate shell executable setting.

The docstring succinctly explains the purpose of the file, which is good for maintainability. Setting the shell executable to "bash" is a common and appropriate practice in Snakefiles.

---

7-8: Consider adding a name and output/action to the first rule.

While Snakemake allows unnamed rules, adding a name would improve readability and make it easier to reference this rule. Additionally, this rule only specifies an input, which is unusual. Typically, rules have at least an output or action. Consider adding these elements to make the rule's purpose clearer.

To ensure this rule is intentional and not missing any elements, you can run the following command:

This will help verify if this input file is used elsewhere in the workflow.

tests/test_params_outdated_code/.snakemake/metadata/c29tZWRpci90ZXN0Lm91dA== (4)

1-1: Verify timestamp accuracy

The starttime and endtime values (1728902332.536915 and 1728902332.541915) correspond to dates in 2024, which is in the future. This might indicate an issue with the system clock or how timestamps are being generated.

To check if this is an isolated incident or a pattern, run:

#!/bin/bash
# Search for timestamps in other metadata files
rg --type json '"starttime":\s*\d+\.\d+' .snakemake/metadata | head -n 10

---

1-1: Review empty input array and checksums

The input array and input_checksums object are both empty. While this might be intentional, it's worth verifying:

1. If the rule truly doesn't require any input files.
2. If input file integrity checks are being skipped unintentionally.

To check if this is common in other metadata files, run:

#!/bin/bash
# Count metadata files with empty input arrays and checksums
echo "Files with empty input:"
rg --type json '"input":\s*\[\]' .snakemake/metadata | wc -l
echo "Files with empty input_checksums:"
rg --type json '"input_checksums":\s*\{\}' .snakemake/metadata | wc -l

---

1-1: ⚠️ Potential issue

Review parameter formatting

The params array contains strings with single quotes: ["'-f'", "'somedir'"]. This unusual formatting might cause issues when these parameters are used.

Consider removing the single quotes:

-"params": ["'-f'", "'somedir'"],
+"params": ["-f", "somedir"],

To check if this is a consistent pattern in the codebase, run:

✅ Verification successful

Review Parameter Formatting Verified

The params array formatting issue is present in the specified file, and no similar patterns were found in other metadata files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar parameter formatting in other metadata files
rg --type json '"params":\s*\[.*'.*'.*\]' .snakemake/metadata

Length of output: 393

---

1-1: ⚠️ Potential issue

Caution: Potentially risky shell command

The shell command rm -r somedir; mkdir -p somedir; touch -f somedir/test.out involves removing and recreating a directory. This operation can be risky if not handled carefully, especially in a production environment.

Consider the following suggestions:

1. Use safer alternatives like rm -rf to suppress errors if the directory doesn't exist.
2. Add error checking or use set -e to ensure the script stops if any command fails.
3. Consider using mktemp for creating temporary directories to avoid potential conflicts.

To verify the usage of this command across the codebase, run:

✅ Verification successful

Verification Completed: No Widespread Risk Identified

The search for rm -r commands revealed usage only in Dockerfile and Makefile files, which are appropriate contexts for such operations. No instances were found in Python files or other parts of the main codebase.

• The rm -r command in the reviewed Snakemake metadata file appears to be an isolated case.
• Ensure that such shell commands are used cautiously and are necessary for the workflow.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar rm -r commands in the codebase
rg --type python "rm\s+-r"

Length of output: 59

---

Script:

#!/bin/bash
# Search for similar rm -r commands in all files
rg "rm\s+-r"

# Alternatively, search specifically in Python files by extension
rg "rm\s+-r" --glob "*.py"

Length of output: 198

tests/tests.py (1)

16-16: Import statement added correctly.

The new import statement from snakemake.persistence import Persistence is correctly placed and necessary for the new test function.