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

Removed all SBC ShARC configs/docs and replaced with TUoS Stanage #552

Closed
wants to merge 5 commits into from
Closed

Removed all SBC ShARC configs/docs and replaced with TUoS Stanage #552

wants to merge 5 commits into from

Conversation

lquayle88
Copy link
Contributor


name: Config for TUoS Stanage
about: Replacement for SBC ShARC

Please follow these steps before submitting your PR:

  • If your PR is a work in progress, include [WIP] in its title
  • Your PR targets the master branch
  • You've included links to relevant issues, if any
  • Requested review from @nf-core/maintainers and/or #request-review on slack

Steps for adding a new config profile:

  • Add your custom config file to the conf/ directory
  • Add your documentation file to the docs/ directory
  • Add your custom profile to the nfcore_custom.config file in the top-level directory
  • Add your custom profile to the README.md file in the top-level directory
  • Add your profile name to the profile: scope in .github/workflows/main.yml

@pontus
Copy link
Contributor

pontus commented Sep 16, 2023

I see they were there before, but what's the purpose of all those pipeline specific configs? Without checking it looks like the standard config has just been brought in which seems to be a less than great idea - if any, can you please just include the bits that differ (and since you know the resources available, no need for check_max and so on).

@lquayle88
Copy link
Contributor Author

They have been modified from the base config and aren't just a direct copy and are the resource requests that have been tested for each pipeline. They were there before for the previous cluster for the same reason. We have had a number of times where a pipeline version has changed and has stopped working due to the pipeline resources changing, so we have started locking the versions we use. Check_max is included as, for whatever reason, if it isn't included here automatic retry doesn't work.

@pontus
Copy link
Contributor

pontus commented Sep 17, 2023

I took a look and am not convinced:

  • atacseq, chipseq and smrnaseq don't seem to load any profile-pipeline-specific configuration so those files don't seem to be used
  • sarek and rnaseq seem to be the same as their base configurations minus maxRetries/maxErrors (which will be set already by the base so no difference).
  • there's no sensible reason I can think of that resource requirements would differ widely on a system basis - it can differ very widely with the data used, but that doesn't seem to be what this is about

"Locking versions" (e.g. deciding for one version to be used consistently within a project) is likely a good idea most of the time, but that is another thing from having profile specific configurations for pipelines - having such those rather makes it more likely there will be problems, especially if it's a copy of all settings rather than bringing in specific settings one want to address for whatever reason.

(Example: sarek sees that fastp rather makes it through with 4 Gbyte of RAM and changes the configuration for that to default to multiples of 6 Gbytes, but as there's a pipeline specific configuration overriding that, those that run with that profile don't get the benefit of that but either need to tune themselves or let their jobs fail with the lower setting and then be retried.)

@lquayle88
Copy link
Contributor Author

I have removed all pipeline specific configurations and related documents. If the requirements for a PR to be approved are more fastidious than the checklist provided, perhaps it needs an update 🤔

@pontus
Copy link
Contributor

pontus commented Sep 17, 2023

Notice that I didn't require changes :)

I understand if it seems odd getting these comments now if you didn't the last time, but the configs are pretty dependent on someone using the sites to submit updates when needed, and a lot of the time it's probably just reviewed favourably if nothing sticks out.

As for these, as they don't differ, they don't break anything (now), they just don't seem like a good idea at all to me (e.g. can be confusing).

conf/tuos_stanage.config Outdated Show resolved Hide resolved
@lquayle88 lquayle88 closed this Sep 17, 2023
@lquayle88 lquayle88 reopened this Sep 17, 2023
@lquayle88 lquayle88 closed this Sep 17, 2023
@lquayle88 lquayle88 deleted the tuos_stanage branch September 17, 2023 16:55
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