Skip to content

chore: updates to satisfy ruff and sprocket lint - #322

Merged
adthrasher merged 6 commits into
mainfrom
chore/python_updates_ruff
Aug 3, 2026
Merged

chore: updates to satisfy ruff and sprocket lint#322
adthrasher merged 6 commits into
mainfrom
chore/python_updates_ruff

Conversation

@adthrasher

@adthrasher adthrasher commented Aug 3, 2026

Copy link
Copy Markdown
Member

Updates to satisfy the new version of Ruff.

Adds set to each command block to satisfy sprocket lint.

Before submitting this PR, please make sure:

  • You have added a few sentences describing the PR here.
  • The code passes all CI tests without any errors or warnings.
  • You have added tests (when appropriate).
  • You have added an entry in any relevant CHANGELOGs (when appropriate).
  • If you have made any changes to the scripts/ or docker/ directories, please ensure any image versions have been incremented accordingly!
  • You have updated the README or other documentation to account for these changes (when appropriate).

@adthrasher
adthrasher requested a review from a-frantz August 3, 2026 15:31
@adthrasher adthrasher changed the title Chore/python updates ruff chore: python updates for ruff Aug 3, 2026
@adthrasher

Copy link
Copy Markdown
Member Author

The Sprocket lint appears to be failing for the new set rule. We have historically disagreed with putting set in simple command blocks, but maybe we should reevaluate.

As for the image bumps, this PR doesn't actually change anything, so I'm inclined not to bump the images and re-release. I think we can just update the script formatting and let that change trickle out as we make substantive changes to the images.

@adthrasher adthrasher self-assigned this Aug 3, 2026

@a-frantz a-frantz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

noting that this PR does not include any CHANGELOG entries, and I think that's fine. Just one question about a java flag, otherwise LGTM

Comment thread tools/gatk4.wdl
gatk \
--java-options \
"-XX:GCTimeLimit=50 -XX:GCHeapFreeLimit=10 -Xms3000m -Xmx~{java_heap_size}g" \
"-XX:GCTimeLimit=50 -XX:GCHeapFreeLimit=10 -Xms3000m -Xmx~{java_heap_size}g -XX:-UseContainerSupport" \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what's this flag?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I shared this in Slack previously (while you were out): https://stjude.slack.com/archives/C025KBTS9JQ/p1778606767391569

Essentially, it looks like, by default, the JVM detects when it is running in a container and can override your heap space settings. This seems to only really be an issue when using small heap spaces, such as during our CI tests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was previously buried in #282, but I promoted it to this PR since it resolves the CI test failures in gatk4.

@a-frantz a-frantz changed the title chore: python updates for ruff chore: updates to satisfy ruff and sprocket lint Aug 3, 2026
@adthrasher
adthrasher merged commit 87a50a0 into main Aug 3, 2026
61 checks passed
@adthrasher
adthrasher deleted the chore/python_updates_ruff branch August 3, 2026 18:50
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.

2 participants