Skip to content

Change statement joining in mapping and preprocess. #165

Merged
Acribbs merged 22 commits intomasterfrom
{IS}_change_command_join
Sep 23, 2025
Merged

Change statement joining in mapping and preprocess. #165
Acribbs merged 22 commits intomasterfrom
{IS}_change_command_join

Conversation

@IanSudbery
Copy link
Copy Markdown
Contributor

Removed ; from mapping.py and preprocess.py and added &&s where necce…ssary

@IanSudbery
Copy link
Copy Markdown
Contributor Author

I feel like I should be testing this, but I don't know how to use the testing framework for the pipelines, if anyone can enlighten me...

@Acribbs
Copy link
Copy Markdown
Contributor

Acribbs commented Jun 23, 2025

The pipelines used to be tested locally because of their size, this isnt happening now. The testing framework for this is so old that it would require significant refactoring. You could write a test to just check for the presence of semi colons, but looking at the failed github action tests, the runners are incredibly outdated and would require some work to use latest environments

@IanSudbery
Copy link
Copy Markdown
Contributor Author

I can obviously easily run a few things (say STAR/BWA/Fastqc/cutadapt/trimmomatic), but I'm not sure I feel up to running an exhaustive list of every mapper and every read processor, with every option, on every data type to make sure it is all consistent.

IanSudbery and others added 2 commits July 8, 2025 17:56
Workflows requires action cache to be v4. I don't really know what this means, but I updated to v4, and lets see.
@Acribbs
Copy link
Copy Markdown
Contributor

Acribbs commented Jul 8, 2025

From the failures here, it looks like setup-miniconda is managing the env and the mambe env update creates issues. Might be worth removing instance of mamba env update and relying on setup-miniconda

@Acribbs
Copy link
Copy Markdown
Contributor

Acribbs commented Jul 8, 2025

something like this (chatgpt so double check):

- name: Set up Miniconda with Mamba
  uses: conda-incubator/setup-miniconda@v2
  with:
    miniconda-version: "latest"
    python-version: ${{ matrix.python-version }}
    channels: conda-forge, bioconda, defaults
    channel-priority: true
    mamba-version: "*"
    use-mamba: true
    activate-environment: cgat-flow
    environment-file: conda/environments/cgat-flow-pipelines.yml
    auto-update-conda: true
    use-only-tar-bz2: true

@Acribbs
Copy link
Copy Markdown
Contributor

Acribbs commented Sep 22, 2025

@IanSudbery This now passes, but the whole repo is a bit of a mess, I have updated the github actions so the runners at least work, but a lot of import tests were failing because the latest python version have a lot of deprecations. This would likely take a lot of effort to get all pipelines fully up to scratch. You happy to merge?

@IanSudbery
Copy link
Copy Markdown
Contributor Author

Yes. Are we going to make a descision that the pipelines are no longer maintained? Or hive off something easier to maintain that will be maintained, and bits that won't. Its something that should probably be done at some point.

We still use the readqc and mapping pipelines, occasionally the bamstats pipeline but pretty muhc nothing else.

@Acribbs
Copy link
Copy Markdown
Contributor

Acribbs commented Sep 22, 2025

I would suggest we significantly reduce the pipelines. Down to its absolute minimum

@IanSudbery
Copy link
Copy Markdown
Contributor Author

I agree. For me the framework is more important than the final pipelines. But I guess this is a decision we can't make just the two of us.

@Acribbs
Copy link
Copy Markdown
Contributor

Acribbs commented Sep 23, 2025

Is anyone else using these pipelines though?

@Acribbs
Copy link
Copy Markdown
Contributor

Acribbs commented Sep 23, 2025

likely a seperate issue, I will merge for the time being and we can come back to this

@Acribbs Acribbs merged commit 0567bf1 into master Sep 23, 2025
4 checks passed
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