Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop removing periods from the end of output names #1433

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

tlangs
Copy link
Contributor

@tlangs tlangs commented Nov 22, 2019

Description

CollectMultipleMetrics currently strips a trailing period from the OUTPUT parameter. This behavior is unexpected and causes problems in our cloud pipelines. Metrics files, by convention, are names ${sample_alias}.${metrics_suffix}. In removing a trailing . from the OUTPUT, CollectMultipleMetrics writes unexpected file names, causing problems when our cloud pipelines try to delocalize files. When supplied an OUTPUT of foo.bar., the resulting metrics files should look like foo.bar..pre_adapter_detail_metrics. This is still a valid path in all filesystems.


Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

@tlangs tlangs requested review from gbggrant and yfarjoun November 22, 2019 14:45
Copy link
Contributor

@gbggrant gbggrant left a comment

Choose a reason for hiding this comment

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

Looks good. I think this is the correct behavior. @yfarjoun might want to take a look to see if there are any edge cases you are not aware of.

@yfarjoun
Copy link
Contributor

I think that this requires more as this could be a breaking change for many users....since the user of CollectMultipleMetrics has no idea that the program will add the suffix with a . as opposed to without. I agree that a consistent behavior is better, especially for consistent pipelines that might have a period at the end of a sample-name (why-oh-why?!!?)

Perhaps each "Program" could include in its description the list of suffixes that it will use? This can be done by having Program implement ClpEnum.

@yfarjoun
Copy link
Contributor

I actually did the ClpEnum thing now in #1555 so @tlangs do you want to re-evaluate the importance of this PR?

@kbergin
Copy link
Contributor

kbergin commented Feb 22, 2021

@tlangs I recall us going through some iterations of this being implemented/causing prod issues/getting fixed last year. I'm going through open picard prs now, is this PR still relevant?

@jessicaway
Copy link
Member

@tlangs Are we good to close this PR?

@lbergelson lbergelson force-pushed the tl_respect_user_output_choice branch from 949c533 to 185118b Compare November 15, 2022 18:15
@tmelman tmelman merged commit 5295289 into master Nov 15, 2022
@tmelman tmelman deleted the tl_respect_user_output_choice branch November 15, 2022 18:41
@tmelman tmelman restored the tl_respect_user_output_choice branch November 15, 2022 18:42
tmelman added a commit that referenced this pull request Nov 15, 2022
@kachulis kachulis deleted the tl_respect_user_output_choice branch November 16, 2022 15:41
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.

6 participants