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

Import commit history from slam_3UIs #46

Merged
merged 63 commits into from
Nov 20, 2024
Merged

Conversation

ns-rse
Copy link
Contributor

@ns-rse ns-rse commented Nov 20, 2024

I didn't want to lose the commit history from the jjriley1/slam_3UIs.

There have been a few commits on a branch @jjriley1 made (jjriley1/1-tidy-up) and I forked from it to make a few changes myself (on branch ns-rse/reprex) and it is this branch that I wanted to pull in.

I therefore undertook the following on ns-rse/slam_3UIs-import to bring the history in...

git remote add slam_3UIs [email protected]:jjriley1/slam_3UIs.git   ## Add a second (old) remote
git fetch slam_3UIs  ## Fetch the references for the second (old) remote
git switch -c ns-rse/slam_3UIs-import ## Create a branch to pull the changes into
git pull slam_3UIs ns-rse/reprex   ## Pull the branch of interest from the second (old) remote

This resulted in some merge conflicts which were easy to resolve (both repositories had a README.md).

However, I couldn't merge ns-rse/slam_3UIs-import into main because they had unrelated histories.

Not wanting to mess anything up having done the import I therefore created a new branch ns-rse/31-import-slam_3UIs from the HEAD of ns-rse/slam_3UIs-import and merged main into it using...

git switch -c ns-rse/31-import-slam_3UIs
git merge main --allow-unrelated-histories
git push

...I can now make this Pull Request to main.

NB I couldn't do

git switch main
git merge ns-rse/slam_3UIs-import --allow-unrelated-histories
git push 

...because we have a branch protection rule on main which requires pull requests and also a pre-commit hook (no-commit-to-branch) which prevents committing directly to main/master.

IanSudbery and others added 30 commits August 29, 2023 12:40
Importing all components of a package/module via `*` imports is an
[anti-pattern](https://www.geeksforgeeks.org/why-import-star-in-python-is-a-bad-idea/). From the perspective of @ns-rse
who has never used `ruffus` before it makes it hard for me to read and understand what functions come from where and in
turn to look up documentation when needed. E.g. I'd never come across `regex()` function before, the standard library
uses `re()` so I wasn't sure what was different about it nor how to look it up.

I've therefore switched to `import ruffus` and through trial and error prefixed all decorators and functions with
`ruffus.` to make it explicit where these are coming from.

I've also applied some basic formatting (removing blank spaces, wrapping lines) so it looks like a lot of changes but
many are just layout.
Closes #6

Includes the [Contributor Covenant Code of Conduct (verbatim)](https://www.contributor-covenant.org/).

I've included both myself and @IanSudbery as contacts in the event of issues.
Closes #4

However `README.md` _will_ evolve over time.
Closes #13

Currently I've not been able to find the requirements/dependencies used in Conda environments I've been pointed to but
will update those once I've identified them.
Closes #11

May revise in light of additinal information that users can provide when errors arise (e.g. FASTA formats, STAR
options), for now its fairly generic.
Current error message is...

```
body[8]: default index is larger that number of options.
```

So I think its because I set the default options for `dropdown` beyond the range of listed items (forgetting that
indexing starts at 0).
Closes #15

Nothing to build yet and requires configuration from [readthedocs.org](https://readthedocs.org) side to actually deploy.
I've looked through the imports in the package itself and the list seems considerably smaller than those from the Conda
environment. This isn't surprising as there is always tons of dependencies that get pulled in.

I searched for import in the existing code and found the unique ones...

```bash
❱ grep -R "import " | awk -F':' '{ print $3 }' | sort | uniq
from cgatcore import pipeline as P
from collections import Counter
from collections import defaultdict
from heapq import merge
from matplotlib import pyplot as plt
from ruffus import *
from statistics import mean
from statistics import median
import cgatcore.experiment as E
import cgatcore.iotools as iotools
import cgatcore.iotools as IOTools
import cgat.GTF as GTF
import cgat.GTF as GTF
import csv
import fnmatch
import glob
import gzip
import io
import os
import pandas as pd
import pysam as pysam
import re
import sqlite3
import sys
```

Many are core libraries but I think I've narrowed down external dependencies to the changes in this commit. I'm sure
I'll find out once I start migrating code if there is anything missing!
Adds configuration files for each of the following...

- [pre-commit](https://pre-commit.com)
- [markdownlint-cli2](https://github.com/DavidAnson/markdownlint-cli2)
- [pylint](https://www.pylint.org)

Linting existing code/files will be done separately.
Closes #14

- Sets up Sphinx documentation under `docs/source`.
- Skeleton documents written in Markdown in place for...
  - `introduction.md`
  - `installation.md`
  - `usage.md`
  - `contributing.md`
  - `workflow.md`
- Will build API documentation automatically via [Sphinx AutoAPI](https://sphinx-autoapi.readthedocs.io/en/latest/).
- Configuration is via `docs/source/conf.py`.
- Uses the [pydata sphinx theme](https://pydata-sphinx-theme.readthedocs.io/en/stable/index.html).
- Builds multiple versions based on tags beginning with `v*` and `main` using [Sphinx
  Multiversion](https://sphinx-contrib.github.io/multiversion/main/index.html)

This commit also disables some packages from `dependencies` as they are failing to install and package dependency will
be undertaken as part of #33.
Pipeline failed as no `conf.py` is in `docs`, this is because its nested under `docs/source/conf.py`

Therefore adding `multiversionopts` for the `source` and `../build` directory. This builds successfully when `ssh`ing
into a `tmate` session.
Closes #7

Also adds a link for [FAIR-Software.eu](https://fair-software.eu/) to the top (missing a PyPI badge which won't happen
for a while and a checklist file)
This runs the code on the minimal reproducible example that @jjriley1 is developing on
[jjriley1/slam_3UIs:jjriley1/1-tidy-up](https://github.com/jjriley1/slam_3UIs/tree/jjriley1/1-tidy-up).

Installing in a clean virtual environment the script at least runs against commit
`0d00ec98253919a3cf5a4b34e5e6bde771acfb1e` (and `fb93e864390e2a3517ffb7ee2bed5ab2cc923a9c`) although currently because
of hard coded files in `pipeline_slam_3UIs/pipeline.yaml` no processing is undertaken with the sample files that have
been provided.

I expect the dependencies to grow as we add more functionality and have deliberately left `pandas`, `numpy` and
`matplotlib` as dependencies as they will be used to do the legwork that the various R scripts currently do.
Closes #10

Adds a workflow to run tests on GitHub Actions Continuous Integration.

It only runs `pull_request` for certain events and even more specifically on changes to specific files (see the `paths`
list under `pull_request`).

At the same time I've linked the `sudlab/IsoSLAM` repository to [codecov.io](https://codecov.io) so that the tests,
which generate code coverage reports (in XML) are uploaded and available for analysis automatically. Required adding a
Secret Token from Codecov to the repository.

May be tweaked in future but this is a good starting point.
@jjriley1 provided a minimal reproducible example to on which regression tests can be made in the `jjriley1/slam_3UIs`
on the `jjriley1/1-tidy-up` branch. @ns-rse made a few tweaks to get it working locally for him.

In order to preserve git history from this repository we rebased onto that using the following and moved the files around.

```bash
git remote add slam_3UIs [email protected]:jjriley1/slam_3UIs.git
git fetch slam_3UIs
git switch -c ns-rse/slam_3UIs-import
git pull slam_3UIs ns-rse/reprex
```

There were some merge conflicts to resolve as both repositories had a `README.md` and the
`.github/ISSUE_TEMPLATE/bug_report.yaml` also raised one but these were easily resolved.

Finally we move the existing pipeline into the package structure ready for tests to be added prior to refactoring.

```bash
git mv pipeline_slam_3UIs.py isoslam/.
git mv pipeline_slam_3UIs isoslam/.
```
@ns-rse
Copy link
Contributor Author

ns-rse commented Nov 20, 2024

NB Entirely expected that...

  • pre-commit checks fail as the code as it stands is a long way off being PEP8 compliant.
  • tests fails as they've not been written yet!

@ns-rse ns-rse merged commit 31cf0b9 into main Nov 20, 2024
1 of 14 checks passed
@ns-rse ns-rse deleted the ns-rse/31-import-slam_3UIs branch November 20, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants