Skip to content

Conversation

@fgvieira
Copy link
Collaborator

@fgvieira fgvieira commented Oct 10, 2025

QC

While the contributions guidelines are more extensive, please particularly ensure that:

  • test.py was updated to call any added or updated example rules in a Snakefile
  • input: and output: file paths in the rules can be chosen arbitrarily
  • wherever possible, command line arguments are inferred and set automatically (e.g. based on file extensions in input: or output:)
  • temporary files are either written to a unique hidden folder in the working directory, or (better) stored where the Python function tempfile.gettempdir() points to
  • the meta.yaml contains a link to the documentation of the respective tool or command under url:
  • conda environments use a minimal amount of channels and packages, in recommended ordering

Summary by CodeRabbit

  • Chores
    • Removed explicit Python version pins from multiple GATK3 environments.
    • Removed samtools from Paladin index and prepare environments.
    • Removed numpy, pandas and polars from the datavzrd environment.
    • Updated Conda tooling metadata to 25.9.0 and reduced explicit third‑party package pins in several manifests.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

📝 Walkthrough

Walkthrough

Removed explicit Python (python=3.14.0) pins from several GATK3 environment files, removed samtools from Paladin environments, deleted numpy/pandas/polars from the datavzrd environment, and pruned many entries and updated conda metadata in multiple pinned environment manifests.

Changes

Cohort / File(s) Summary
GATK3 Python dependency removals
bio/gatk3/baserecalibrator/environment.yaml, bio/gatk3/indelrealigner/environment.yaml, bio/gatk3/printreads/environment.yaml, bio/gatk3/realignertargetcreator/environment.yaml
Removed explicit python =3.14.0 pin from dependencies in each GATK3 environment YAML.
Paladin samtools removal
bio/paladin/index/environment.yaml, bio/paladin/prepare/environment.yaml
Deleted samtools from Paladin environment YAML dependencies.
Paladin pinned environment cleanup
bio/paladin/index/environment.linux-64.pin.txt, bio/paladin/prepare/environment.linux-64.pin.txt
Updated created-by conda metadata (24.9.2 → 25.9.0) and removed several explicit pinned artifacts (bzip2, libdeflate, liblzma, htslib, samtools, etc.) from the Linux-64 pin manifests.
DataVzrd dependency removals
utils/datavzrd/environment.yaml
Removed numpy, pandas, and polars from the datavzrd environment YAML dependencies.
DataVzrd pinned manifest pruning
utils/datavzrd/environment.linux-64.pin.txt
Removed numerous platform-specific and Python/ecosystem pins (numpy, pandas, libgfortran variants, BLAS/LAPACK, tzdata, python-dateutil, polars, etc.) from the Linux-64 pin manifest.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Multiple environment YAMLs and pinned manifests changed; changes are repetitive but affect reproducibility and dependency matrices.
  • Pay extra attention to:
    • Compatibility implications of removing Python pins in GATK3 wrappers.
    • Downstream consumers expecting samtools in Paladin environments.
    • Datavzrd removal of numpy/pandas/polars and the large pin pruning in environment.linux-64.pin.txt.

Possibly related PRs

Suggested labels

bio/gatk3/printreads

Suggested reviewers

  • johanneskoester
  • cmeesters

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description is largely incomplete despite following the template structure. The main descriptive section—where the author should explain what dependencies are being removed and why they are unnecessary—is entirely empty, with only the template placeholder comments visible. While the QC checklist is fully completed with all boxes checked, most of these items are not applicable to this PR about environment configuration changes (e.g., "test.py was updated," "input:/output: file paths can be chosen arbitrarily," "temporary files are stored appropriately," and "meta.yaml contains documentation links" are not relevant to removing environment dependencies). The description lacks substantive information about the rationale for removing these specific dependencies or any technical justification for the changes. Add a meaningful description explaining which dependencies are being removed, why they are unnecessary, and any relevant context or testing performed. Also review the QC checklist to ensure only applicable items are marked as completed; several items in the list do not pertain to environment configuration changes. The description should provide enough detail that reviewers understand the motivation and scope of this dependency cleanup effort.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: remove unecessary deps" follows the conventional commit style with "fix:" as the type prefix and provides a clear, concise description of the main change. The changeset demonstrates exactly what the title claims: removing dependencies across multiple environment files (Python 3.14.0 from gatk3 modules, samtools from paladin modules, and numpy/pandas/polars from datavzrd). Despite a minor typo ("unecessary" instead of "unnecessary"), the title is specific enough that a teammate reviewing the commit history would quickly understand the primary objective of this PR. The title accurately summarizes the changeset and relates directly to the actual modifications.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 17ef091 and 9e694c0.

📒 Files selected for processing (1)
  • utils/datavzrd/environment.linux-64.pin.txt (0 hunks)
💤 Files with no reviewable changes (1)
  • utils/datavzrd/environment.linux-64.pin.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: testing
  • GitHub Check: docs
  • GitHub Check: Summary

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@fgvieira fgvieira requested a review from dlaehnemann October 10, 2025 08:30
Comment on lines -7 to -9
- numpy
- pandas
- polars
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxwiegand I think these were added intentionally weren't they?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests do not fail and, if they are needed by datavzrd, shouldn't they be added directly on the conda recipe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were added because of this template rendering issue with YTE that complained about something with numpy I think. @johanneskoester is this solved now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they have to be present whenever the datavzrd config that is passed to the wrapper is a YTE template that contains numpy, pandas, or polars elements. Before we remove them, we should definitely extend the test case in this repo to also do that, maybe in a followup PR instead of changing it here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants