Versioning of algos support - #82
Conversation
…ned algo structure, add script for migration of the current structure
📝 WalkthroughWalkthroughAdds version-aware outputs and tooling: new version utilities, a migration script, run script and evaluation updates to produce and consume outputs under Changes
Sequence Diagram(s)sequenceDiagram
participant Run as run.sh
participant VersionUtils as version_utils
participant FS as Filesystem
participant Augment as augment_predictions.py
participant Eval as evaluate.py
Run->>VersionUtils: get_all_algorithms_versions(outputs_root)
VersionUtils->>FS: read versions.log for each algorithm
FS-->>VersionUtils: versions metadata
VersionUtils-->>Run: map of algorithm → version
Run->>FS: create outputs/{algo}/{version}/ (per-algo)
Run->>VersionUtils: create_latest_symlink(outputs, algo, version)
VersionUtils->>FS: create/update outputs/{algo}/latest -> {version}
Run->>Augment: execute with --dataset_name
Augment->>FS: write outputs/{algo}/{version}/{dataset}_output.csv
Run->>Eval: execute with --dataset_name, outputs_root
Eval->>FS: read outputs/{algo}/{version}/{dataset}_output.csv
Eval->>Eval: compute metrics labeled by display_name ({algo}_{version}[_latest])
Eval-->>Run: aggregated results and plots
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
evaluation/evaluate.py (1)
248-276: Critical: Evaluation logic is outside the version loop.The processing code starting at line 269 is not indented within the
for version in os.listdir(algo_dir)loop. This means only the last algorithm/version combination will be evaluated, and all others will be silently skipped.All code from line 269 to approximately line 454 should be indented to be inside the version loop.
Proposed fix (showing the loop structure)
output_path = os.path.join(version_dir, output_file) if not os.path.isfile(output_path): continue # Determine display name: {algo}_{version} or {algo}_{version}_latest is_latest = (version == latest_version) display_name = f"{algo_name}_{version}_latest" if is_latest else f"{algo_name}_{version}" print(f"EVALUATE {display_name}") # Load tool predictions, match with ground truth output_data = utils.load_predictions(output_path, sequences_true) - # Get idxs of GT labeled peptides & sequenced peptides (in correct output format) - print(display_name) + # Get idxs of GT labeled peptides & sequenced peptides (in correct output format) + print(display_name)Continue indenting all subsequent lines through the debug output at line 454.
🤖 Fix all issues with AI agents
In `@migrate_to_versioned_structure.py`:
- Around line 29-68: discover_old_structure_files currently returns a 4-tuple
(file_path, algorithm, dataset_name, suffix) but its type annotation and
docstring describe a 3-tuple; update the signature and docstring to reflect the
fourth element or remove the suffix from the returned tuples — e.g., change the
return type to List[Tuple[Path, str, str, str]] and update the docstring to list
the four-tuple (file_path, algorithm_name, dataset_name, suffix) so the
annotation, docstring, and actual return value match (refer to
discover_old_structure_files and the files.append call that produces the
4-tuple).
In `@version_utils.py`:
- Around line 37-42: get_latest_version currently assumes parse_versions_log
returned a non-empty list whose first dict contains 'container_version', which
can raise KeyError or IndexError; update get_latest_version to validate the
result of parse_versions_log: ensure the returned list is non-empty and that the
first item (or each item) contains the 'container_version' key (use dict.get to
check), and if not present raise a clear ValueError (or return a documented
fallback) with context including the algorithm_path and the offending entry;
reference the function name get_latest_version and the key 'container_version'
when implementing the check and error.
🧹 Nitpick comments (8)
version_utils.py (1)
70-75: Consider narrowing the exception type.The broad
Exceptioncatch is acceptable here for bulk scanning with graceful degradation. For improved clarity, consider catching specific exceptions like(KeyError, ValueError, FileNotFoundError)thatget_latest_versionandparse_versions_logmay raise.run.sh (3)
67-72: Version extraction duplicated and lacks error handling.The Python inline command for extracting
algo_versionis duplicated in both algorithm loops (here and at lines 143-147). If the YAML parsing fails (e.g., malformed file), the script will fail silently or setalgo_versionto an error message.Consider extracting this to a shell function or using the Python
version_utilsmodule consistently. At minimum, add error handling:Proposed fix with error handling
# Get algorithm version from versions.log if [ -f "$algorithm_dir/versions.log" ]; then - algo_version=$(python3 -c "import yaml; data=yaml.safe_load(open('$algorithm_dir/versions.log')); print(data[0]['container_version'])") + algo_version=$(python3 -c "import yaml; data=yaml.safe_load(open('$algorithm_dir/versions.log')); print(data[0]['container_version'])" 2>/dev/null) + if [ -z "$algo_version" ]; then + echo "Warning: Could not parse version for $algorithm_name, using 'unknown'" + algo_version="unknown" + fi else algo_version="unknown" fi
78-85: Missing validation that version directory exists before creating symlink.The symlink is created without first verifying that
$algo_output_direxists. Whilemkdir -pon line 75 should have created it, adding a check would be more robust and consistent withversion_utils.create_latest_symlink.Proposed fix
# Create/update 'latest' symlink pointing to current version if [ "$create_symlinks" = true ]; then latest_link="$output_root_dir/$algorithm_name/latest" - if [ -L "$latest_link" ]; then - rm "$latest_link" - fi - ln -s "$algo_version" "$latest_link" + if [ -d "$algo_output_dir" ]; then + if [ -L "$latest_link" ] || [ -e "$latest_link" ]; then + rm "$latest_link" + fi + ln -s "$algo_version" "$latest_link" + fi fi
142-150: Duplicated version extraction logic.This version extraction block is identical to lines 68-72. Consider refactoring into a shell function to reduce duplication and ensure consistent behavior.
Proposed refactor with shell function
Add this function near the top of the script after variable declarations:
get_algo_version() { local algorithm_dir="$1" if [ -f "$algorithm_dir/versions.log" ]; then python3 -c "import yaml; data=yaml.safe_load(open('$algorithm_dir/versions.log')); print(data[0]['container_version'])" 2>/dev/null || echo "unknown" else echo "unknown" fi }Then replace both extraction blocks with:
algo_version=$(get_algo_version "$algorithm_dir")migrate_to_versioned_structure.py (2)
162-172: Consider narrowing exception type for symlink creation.The broad
Exceptioncatch is acceptable for robustness during migration, but you could narrow it to(FileNotFoundError, OSError)which are the expected failure modes for symlink operations.
191-196: Cleanup may encounter new versioned directories.When iterating
outputs_dir.iterdir()for cleanup, the loop will also encounter newly created algorithm directories (from the versioned structure). The empty check protects against accidental removal, but consider filtering to only process directories that were identified in the old structure:Proposed fix
# Remove empty directories - for dataset_dir in outputs_dir.iterdir(): - if dataset_dir.is_dir() and not any(dataset_dir.iterdir()): - dataset_dir.rmdir() - print(f" Removed empty directory: {dataset_dir.name}") - removed_dirs += 1 + old_dataset_dirs = {file_path.parent for file_path, _, _, _ in old_files} + for dataset_dir in old_dataset_dirs: + if dataset_dir.exists() and dataset_dir.is_dir() and not any(dataset_dir.iterdir()): + dataset_dir.rmdir() + print(f" Removed empty directory: {dataset_dir.name}") + removed_dirs += 1evaluation/evaluate.py (2)
19-21: Consider using relative import or package structure instead ofsys.pathmanipulation.Manipulating
sys.pathat module level is fragile and can cause import issues. Consider either:
- Moving
version_utils.pyinto theevaluationpackage- Using a proper package installation with
setup.pyorpyproject.toml
260-262: Consider simplifying display_name for latest version.The current format
{algo}_{version}_latest(e.g., "casanovo_v1.0.0_latest") is verbose. Since the version is explicit, consider just using{algo}_{version}for all entries and indicating "latest" separately in metrics/reports.Alternatively, if distinguishing latest is important in plots, consider
{algo} (v1.0.0, latest)for better readability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
evaluation/augment_predictions.pyevaluation/evaluate.pymigrate_to_versioned_structure.pyrun.shversion_utils.py
🧰 Additional context used
🧬 Code graph analysis (2)
migrate_to_versioned_structure.py (1)
version_utils.py (2)
get_all_algorithms_versions(45-77)create_latest_symlink(80-102)
evaluation/evaluate.py (2)
version_utils.py (1)
get_latest_version(37-42)evaluation/utils.py (1)
load_predictions(141-153)
🪛 Ruff (0.14.11)
migrate_to_versioned_structure.py
168-168: Do not catch blind exception: Exception
(BLE001)
version_utils.py
20-23: Avoid specifying long messages outside the exception class
(TRY003)
29-32: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
91-94: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
version_utils.py (1)
80-102: LGTM!The symlink creation logic correctly validates the target directory exists, handles existing symlinks, and uses relative paths for portability.
run.sh (1)
163-167: LGTM!The evaluation call correctly passes
output_root_dirand--dataset_nameparameters to align with the new versioned structure.migrate_to_versioned_structure.py (1)
255-305: LGTM!The CLI setup is well-designed with clear documentation, sensible defaults, and helpful examples. The dry-run safety feature is particularly valuable for a migration script.
evaluation/augment_predictions.py (2)
36-48: LGTM!The addition of
--dataset_nameas a required argument cleanly decouples the dataset identification from the directory structure, enabling the versioned output paths.
62-64: LGTM!The output file naming convention
{dataset_name}_output.csvaligns with the new versioned structure used inrun.shandevaluate.py.evaluation/evaluate.py (1)
241-245: Good error handling for version lookup.The try-except block gracefully handles missing or invalid
versions.logfiles, allowing evaluation to continue for other algorithms. Settinglatest_version = Noneand continuing is the right approach.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def discover_old_structure_files(outputs_dir: Path) -> List[Tuple[Path, str, str]]: | ||
| """ | ||
| Discover all output files in the OLD structure. | ||
|
|
||
| Args: | ||
| outputs_dir: Base outputs directory | ||
|
|
||
| Returns: | ||
| List of tuples: (file_path, algorithm_name, dataset_name) | ||
| """ | ||
| files = [] | ||
|
|
||
| if not outputs_dir.exists(): | ||
| print(f"Warning: Directory {outputs_dir} does not exist") | ||
| return files | ||
|
|
||
| for dataset_dir in outputs_dir.iterdir(): | ||
| if not dataset_dir.is_dir(): | ||
| continue | ||
|
|
||
| dataset_name = dataset_dir.name | ||
|
|
||
| # Find all output files in this dataset | ||
| for output_file in dataset_dir.glob("*_output*.csv"): | ||
| filename = output_file.stem | ||
|
|
||
| # Parse algorithm name from filename | ||
| if filename.endswith('_output_augmented'): | ||
| algorithm = filename.replace('_output_augmented', '') | ||
| suffix = '_output_augmented' | ||
| elif filename.endswith('_output'): | ||
| algorithm = filename.replace('_output', '') | ||
| suffix = '_output' | ||
| else: | ||
| print(f"Warning: Unexpected file format: {output_file.name}") | ||
| continue | ||
|
|
||
| files.append((output_file, algorithm, dataset_name, suffix)) | ||
|
|
||
| return files |
There was a problem hiding this comment.
Type annotation doesn't match actual return value.
The function returns a 4-tuple (file_path, algorithm, dataset_name, suffix) on line 66, but the return type annotation on line 29 specifies List[Tuple[Path, str, str]] (3-tuple), and the docstring on line 37 also describes a 3-tuple.
Proposed fix
-def discover_old_structure_files(outputs_dir: Path) -> List[Tuple[Path, str, str]]:
+def discover_old_structure_files(outputs_dir: Path) -> List[Tuple[Path, str, str, str]]:
"""
Discover all output files in the OLD structure.
Args:
outputs_dir: Base outputs directory
Returns:
- List of tuples: (file_path, algorithm_name, dataset_name)
+ List of tuples: (file_path, algorithm_name, dataset_name, suffix)
"""🤖 Prompt for AI Agents
In `@migrate_to_versioned_structure.py` around lines 29 - 68,
discover_old_structure_files currently returns a 4-tuple (file_path, algorithm,
dataset_name, suffix) but its type annotation and docstring describe a 3-tuple;
update the signature and docstring to reflect the fourth element or remove the
suffix from the returned tuples — e.g., change the return type to
List[Tuple[Path, str, str, str]] and update the docstring to list the four-tuple
(file_path, algorithm_name, dataset_name, suffix) so the annotation, docstring,
and actual return value match (refer to discover_old_structure_files and the
files.append call that produces the 4-tuple).
| def get_latest_version(algorithm_path: Path) -> str: | ||
| """ | ||
| Get the most recent version from versions.log. | ||
| """ | ||
| versions = parse_versions_log(algorithm_path) | ||
| return versions[0]['container_version'] |
There was a problem hiding this comment.
Potential KeyError if container_version key is missing.
parse_versions_log validates that the file contains a list but doesn't validate that each entry has the container_version key. If the YAML structure is malformed (e.g., missing key or typo), this will raise an unhandled KeyError.
Proposed fix
def get_latest_version(algorithm_path: Path) -> str:
"""
Get the most recent version from versions.log.
"""
versions = parse_versions_log(algorithm_path)
+ if 'container_version' not in versions[0]:
+ raise ValueError(
+ f"Invalid versions.log format in {algorithm_path}. "
+ f"Missing 'container_version' key in first entry."
+ )
return versions[0]['container_version']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_latest_version(algorithm_path: Path) -> str: | |
| """ | |
| Get the most recent version from versions.log. | |
| """ | |
| versions = parse_versions_log(algorithm_path) | |
| return versions[0]['container_version'] | |
| def get_latest_version(algorithm_path: Path) -> str: | |
| """ | |
| Get the most recent version from versions.log. | |
| """ | |
| versions = parse_versions_log(algorithm_path) | |
| if not versions: | |
| raise ValueError( | |
| f"Invalid versions.log format in {algorithm_path}. " | |
| f"File is empty or contains no entries." | |
| ) | |
| if 'container_version' not in versions[0]: | |
| raise ValueError( | |
| f"Invalid versions.log format in {algorithm_path}. " | |
| f"Missing 'container_version' key in first entry." | |
| ) | |
| return versions[0]['container_version'] |
🤖 Prompt for AI Agents
In `@version_utils.py` around lines 37 - 42, get_latest_version currently assumes
parse_versions_log returned a non-empty list whose first dict contains
'container_version', which can raise KeyError or IndexError; update
get_latest_version to validate the result of parse_versions_log: ensure the
returned list is non-empty and that the first item (or each item) contains the
'container_version' key (use dict.get to check), and if not present raise a
clear ValueError (or return a documented fallback) with context including the
algorithm_path and the offending entry; reference the function name
get_latest_version and the key 'container_version' when implementing the check
and error.
07c4fb1 to
d29cd98
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@evaluation/augment_predictions.py`:
- Around line 31-34: The CLI option --dataset_name is used unconditionally
(args.dataset_name) and currently defaults to None, causing tags_df.loc[None]
and incorrect output file names; either make the arg required in the argument
parser (remove default=None and add required=True on the add_argument call that
defines "--dataset_name") or restore the original fallback behavior by adding a
guard where args.dataset_name is checked before use and, if None, iterate over
all algorithms in output_dir (the same logic promised in the help text) and
build CSV paths accordingly; update any code that uses
tags_df.loc[args.dataset_name], the CSV path construction, and the final output
filename logic so they handle both the required-case and the fallback-case
consistently.
In `@evaluation/evaluate.py`:
- Around line 233-267: The metrics/evaluation block must be moved inside the
per-version loop so each version uses its own output_data and display_name
rather than reusing stale values; locate the for version in os.listdir(algo_dir)
loop where utils.load_predictions(output_path, sequences_true) is called and
indent/move the subsequent evaluation/metrics code (the block that references
output_data and display_name) so it executes for every version iteration,
ensuring output_data and display_name are defined in the same scope as the
metrics code and handling the case where output_path is missing by continuing
the loop.
In `@migrate_to_versioned_structure.py`:
- Around line 149-158: The migration loop currently copies files unconditionally
(in the block iterating over files_by_algo[algorithm] and using
new_filename/new_file_path and shutil.copy2), which can overwrite newer
versioned CSVs; change it to first check if new_file_path.exists() and if so
skip copying and log a warning (or, if desired, create a safe backup with a
timestamped name), only performing shutil.copy2 when the destination does not
exist (respecting dry_run behavior by printing the would-copy/skip messages
instead of performing file operations).
In `@run.sh`:
- Around line 153-156: The augmentation call in run.sh passes a stale
--algo_name flag to the module evaluation.augment_predictions which no longer
accepts it; remove the --algo_name ${algorithm_name} argument from the apptainer
exec invocation so python -m evaluation.augment_predictions is invoked only with
--output_dir, --data_dir, and --dataset_name (and any other currently supported
flags), ensuring augment_predictions.py runs past argparse and performs RT/SA
augmentation.
- Around line 38-47: The cleanup under the "$recalculate" branch is too broad:
it iterates algorithms/* and deletes every "${dset_name}_output.csv" and the
whole "$time_log_dir" even when only one algorithm was requested; change it to
restrict deletions to the selected algorithm only by checking the script's
selected-algorithm variable (e.g., the parsed argument used when calling
./run.sh -r) before removing files—inside the loop only run find
"$output_root_dir/$algo_name" -name "${dset_name}_output.csv" -delete for the
matching algo_name (or skip the loop and directly target
"$output_root_dir/$SELECTED_ALGO"), and replace rm -rf "$time_log_dir" with
removal of the per-algorithm time-log (e.g., "$time_log_dir/$SELECTED_ALGO" or
otherwise scoped to algo_name and dset_name) so other tools' results are
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b21b358c-0e94-4885-a58e-f7239e60472f
📒 Files selected for processing (5)
evaluation/augment_predictions.pyevaluation/evaluate.pymigrate_to_versioned_structure.pyrun.shversion_utils.py
| "--dataset_name", | ||
| help="The name of the dataset (used in the output file name). If not provided, " | ||
| "all algorithms in output_dir will be processed.", | ||
| default=None, |
There was a problem hiding this comment.
Make --dataset_name required or restore the fallback.
Line 43 uses args.dataset_name unconditionally to look up dataset tags and build the CSV path, so omitting the flag now resolves to tags_df.loc[None] / None_output.csv. The help text still promises a mode that is no longer implemented.
Suggested fix
parser.add_argument(
"--dataset_name",
- help="The name of the dataset (used in the output file name). If not provided, "
- "all algorithms in output_dir will be processed.",
- default=None,
+ required=True,
+ help="The name of the dataset (used in the output file name).",
)Also applies to: 43-49, 57-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evaluation/augment_predictions.py` around lines 31 - 34, The CLI option
--dataset_name is used unconditionally (args.dataset_name) and currently
defaults to None, causing tags_df.loc[None] and incorrect output file names;
either make the arg required in the argument parser (remove default=None and add
required=True on the add_argument call that defines "--dataset_name") or restore
the original fallback behavior by adding a guard where args.dataset_name is
checked before use and, if None, iterate over all algorithms in output_dir (the
same logic promised in the help text) and build CSV paths accordingly; update
any code that uses tags_df.loc[args.dataset_name], the CSV path construction,
and the final output filename logic so they handle both the required-case and
the fallback-case consistently.
| # Parse versioned structure: outputs/{algo}/{version}/ | ||
| for algo_name in os.listdir(args.output_dir): | ||
| algo_dir = os.path.join(args.output_dir, algo_name) | ||
| if not os.path.isdir(algo_dir): | ||
| continue | ||
|
|
||
| # Determine which version is 'latest' from versions.log | ||
| algo_path = Path("algorithms") / algo_name | ||
| try: | ||
| latest_version = get_latest_version(algo_path) | ||
| except (FileNotFoundError, ValueError) as e: | ||
| print(f"Warning: Could not determine latest version for {algo_name}: {e}") | ||
| latest_version = None | ||
|
|
||
| # Process each version directory | ||
| for version in os.listdir(algo_dir): | ||
| if version == 'latest': # Skip the symlink itself | ||
| continue | ||
| version_dir = os.path.join(algo_dir, version) | ||
| if not os.path.isdir(version_dir): | ||
| continue | ||
|
|
||
| output_file = f"{dataset_name}_output.csv" | ||
| output_path = os.path.join(version_dir, output_file) | ||
| if not os.path.isfile(output_path): | ||
| continue | ||
|
|
||
| # Determine display name: {algo}_{version} or {algo}_{version}_latest | ||
| is_latest = (version == latest_version) | ||
| display_name = f"{algo_name}_{version}_latest" if is_latest else f"{algo_name}_{version}" | ||
|
|
||
| print(f"EVALUATE {display_name}") | ||
|
|
||
| # Load tool predictions, match with ground truth | ||
| output_data = utils.load_predictions(output_path, sequences_true) |
There was a problem hiding this comment.
Keep the metrics block inside the per-version loop.
Only utils.load_predictions() is nested under for version in os.listdir(algo_dir). The evaluation code starting at Line 269 is dedented back to the algorithm loop, so each algorithm only scores the last version that set output_data / display_name, and algorithms with no matching CSV can reuse stale values or hit an unbound variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evaluation/evaluate.py` around lines 233 - 267, The metrics/evaluation block
must be moved inside the per-version loop so each version uses its own
output_data and display_name rather than reusing stale values; locate the for
version in os.listdir(algo_dir) loop where utils.load_predictions(output_path,
sequences_true) is called and indent/move the subsequent evaluation/metrics code
(the block that references output_data and display_name) so it executes for
every version iteration, ensuring output_data and display_name are defined in
the same scope as the metrics code and handling the case where output_path is
missing by continuing the loop.
| for file_path, dataset, suffix in files_by_algo[algorithm]: | ||
| # New filename: {dataset}_output.csv or {dataset}_output_augmented.csv | ||
| new_filename = f"{dataset}{suffix}.csv" | ||
| new_file_path = new_algo_dir / new_filename | ||
|
|
||
| if dry_run: | ||
| print(f" [would copy] {file_path.name} -> {new_file_path.relative_to(outputs_dir)}") | ||
| else: | ||
| shutil.copy2(str(file_path), str(new_file_path)) | ||
| print(f" Copied: {file_path.name} -> {new_file_path.relative_to(outputs_dir)}") |
There was a problem hiding this comment.
Don't overwrite an existing versioned CSV during migration.
The copy is unconditional. Re-running the migration, or migrating into a partially populated outputs/, will silently replace a newer ${dataset}_output.csv with the old flat-layout file, which breaks the script's “copy for safety” guarantee.
Suggested fix
if dry_run:
print(f" [would copy] {file_path.name} -> {new_file_path.relative_to(outputs_dir)}")
else:
+ if new_file_path.exists():
+ print(
+ f" WARNING: Destination already exists, skipping: "
+ f"{new_file_path.relative_to(outputs_dir)}"
+ )
+ continue
shutil.copy2(str(file_path), str(new_file_path))
print(f" Copied: {file_path.name} -> {new_file_path.relative_to(outputs_dir)}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrate_to_versioned_structure.py` around lines 149 - 158, The migration loop
currently copies files unconditionally (in the block iterating over
files_by_algo[algorithm] and using new_filename/new_file_path and shutil.copy2),
which can overwrite newer versioned CSVs; change it to first check if
new_file_path.exists() and if so skip copying and log a warning (or, if desired,
create a safe backup with a timestamped name), only performing shutil.copy2 when
the destination does not exist (respecting dry_run behavior by printing the
would-copy/skip messages instead of performing file operations).
| if "$recalculate"; then | ||
| # Clean output dir | ||
| rm -rf "$output_dir" | ||
| # Clean output and time logs for this dataset | ||
| for algo_dir in algorithms/*; do | ||
| algo_name=$(basename "$algo_dir") | ||
| if [ -d "$algo_dir" ] && [ "$algo_name" != "base" ]; then | ||
| # Remove all versions' outputs for this dataset | ||
| find "$output_root_dir/$algo_name" -name "${dset_name}_output.csv" -delete 2>/dev/null | ||
| fi | ||
| done | ||
| rm -rf "$time_log_dir" |
There was a problem hiding this comment.
Scope -r cleanup to the selected algorithm.
When ./run.sh -r <dataset> <algorithm> is used, this block still deletes every algorithm's ${dset_name}_output.csv and removes the whole dataset time-log directory, but only the selected algorithm is regenerated later. That drops the other tools' results for the dataset.
Suggested fix
if "$recalculate"; then
# Clean output and time logs for this dataset
for algo_dir in algorithms/*; do
algo_name=$(basename "$algo_dir")
- if [ -d "$algo_dir" ] && [ "$algo_name" != "base" ]; then
+ if [ -d "$algo_dir" ] && [ "$algo_name" != "base" ] && { [ -z "$algorithm" ] || [ "$algo_name" = "$algorithm" ]; }; then
# Remove all versions' outputs for this dataset
find "$output_root_dir/$algo_name" -name "${dset_name}_output.csv" -delete 2>/dev/null
fi
done
- rm -rf "$time_log_dir"
+ if [ -z "$algorithm" ]; then
+ rm -rf "$time_log_dir"
+ else
+ rm -f "$time_log_dir/${algorithm}_time.log"
+ fi
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@run.sh` around lines 38 - 47, The cleanup under the "$recalculate" branch is
too broad: it iterates algorithms/* and deletes every "${dset_name}_output.csv"
and the whole "$time_log_dir" even when only one algorithm was requested; change
it to restrict deletions to the selected algorithm only by checking the script's
selected-algorithm variable (e.g., the parsed argument used when calling
./run.sh -r) before removing files—inside the loop only run find
"$output_root_dir/$algo_name" -name "${dset_name}_output.csv" -delete for the
matching algo_name (or skip the loop and directly target
"$output_root_dir/$SELECTED_ALGO"), and replace rm -rf "$time_log_dir" with
removal of the per-algorithm time-log (e.g., "$time_log_dir/$SELECTED_ALGO" or
otherwise scoped to algo_name and dset_name) so other tools' results are
preserved.
| # Augment algorithm predictions with RT and SA (if not already present) | ||
| echo "AUGMENT PREDICTIONS" | ||
| apptainer exec --fakeroot --env-file .env "evaluation.sif" \ | ||
| bash -c "python -m evaluation.augment_predictions --output_dir ${output_dir} --data_dir ${dset_dir} --algo_name ${algorithm_name}" | ||
| bash -c "python -m evaluation.augment_predictions --output_dir ${algo_output_dir} --data_dir ${dset_dir} --algo_name ${algorithm_name} --dataset_name ${dset_name}" |
There was a problem hiding this comment.
Remove the stale --algo_name flag from the augmentation call.
evaluation/augment_predictions.py no longer accepts --algo_name. This invocation now exits in argparse before RT/SA augmentation runs, and the downstream evaluation step then reads CSVs without the expected columns.
Suggested fix
- apptainer exec --fakeroot --env-file .env "evaluation.sif" \
- bash -c "python -m evaluation.augment_predictions --output_dir ${algo_output_dir} --data_dir ${dset_dir} --algo_name ${algorithm_name} --dataset_name ${dset_name}"
+ apptainer exec --fakeroot --env-file .env "evaluation.sif" \
+ bash -c "python -m evaluation.augment_predictions --output_dir ${algo_output_dir} --data_dir ${dset_dir} --dataset_name ${dset_name}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@run.sh` around lines 153 - 156, The augmentation call in run.sh passes a
stale --algo_name flag to the module evaluation.augment_predictions which no
longer accepts it; remove the --algo_name ${algorithm_name} argument from the
apptainer exec invocation so python -m evaluation.augment_predictions is invoked
only with --output_dir, --data_dir, and --dataset_name (and any other currently
supported flags), ensuring augment_predictions.py runs past argparse and
performs RT/SA augmentation.
1 participant
Update run.sh, augment prediction and evaluate.py to work with versioned algo structure, add a script for migration of the current structure
The things still left to address:
run_split.sh is not updated
The dashboard is not updated (but it will work, just the displayed algos will look strange)
I tested everything, but not end-to-end with containers, prediction calculation, and results generation
Summary by CodeRabbit
New Features
Chores