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

Add option to update modules with linting #1588

Merged
merged 38 commits into from
Jun 2, 2022
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
c5c1901
don't updatate sha if the module is not updated
Apr 19, 2022
f26c2d4
modify changelog
Apr 20, 2022
eb972a7
Merge branch 'dev' into update_modules
mirpedrol Apr 20, 2022
f3058b4
Merge branch 'dev' into update_modules
ggabernet May 5, 2022
2374756
don't updatate sha if the module is not updated
Apr 19, 2022
5b04b2b
resolve conflicts in changelog
Apr 20, 2022
e6b1af5
resolve conflicts in main_nf.py
mirpedrol May 16, 2022
cee29f0
fix conflicts
mirpedrol May 17, 2022
21d9a54
get version and build
mirpedrol May 17, 2022
db7fac1
check that url exist & add log info
mirpedrol May 17, 2022
72c49c9
improve logging
mirpedrol May 17, 2022
008350e
improve comments
mirpedrol May 17, 2022
4c87e9d
fix conflicts
mirpedrol May 17, 2022
e152b7b
remove changes made by error
mirpedrol May 17, 2022
52a60ab
merge branch
mirpedrol May 17, 2022
106b60e
update changelog
mirpedrol May 17, 2022
9d839cd
Merge branch 'dev' into update_modules
mirpedrol May 17, 2022
c1c4541
small changelog change
mirpedrol May 17, 2022
e6525ef
improve debug messages
mirpedrol May 19, 2022
faef8ea
Merge branch 'dev' into update_modules
mirpedrol May 30, 2022
0122056
don't updatate sha if the module is not updated
Apr 19, 2022
b288578
resolve conflicts in changelog
Apr 20, 2022
7b1538e
resolve conflicts in main_nf.py
mirpedrol May 16, 2022
ef00ede
fix conflicts
mirpedrol May 17, 2022
f205b15
get version and build
mirpedrol May 17, 2022
877b6f6
resolve rebase conflicts
mirpedrol Jun 2, 2022
3886ad1
improve logging
mirpedrol May 17, 2022
eac514d
improve comments
mirpedrol May 17, 2022
a5ba612
remove changes made by error
mirpedrol May 17, 2022
08e1b94
don't updatate sha if the module is not updated
Apr 19, 2022
99bf430
resolve rebase conflicts
mirpedrol Jun 2, 2022
550f6d2
improve debug messages
mirpedrol May 19, 2022
af8e030
resolve conflicts
mirpedrol Jun 2, 2022
3e9076c
refactor fix flag to fix-version
mirpedrol Jun 2, 2022
8e061f1
fix typo in CHANGELOG.md
mirpedrol Jun 2, 2022
0037bf9
Remove duplicate in CHANGELOG.md
mirpedrol Jun 2, 2022
cfcf3b6
Remove changed made by mistake
mirpedrol Jun 2, 2022
358bb0a
fix black linting
mirpedrol Jun 2, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

### Modules

- Add `fix` flag to `nf-core modules lint` command to update modules to the latest version ([#1588](https://github.com/nf-core/tools/pull/1588))
mirpedrol marked this conversation as resolved.
Show resolved Hide resolved
- Fix a bug in the regex extracting the version from biocontainers URLs [#1598](https://github.com/nf-core/tools/pull/1598)

## [v2.4.1 - Cobolt Koala Patch](https://github.com/nf-core/tools/releases/tag/2.4) - [2022-05-16]
Expand Down
7 changes: 5 additions & 2 deletions nf_core/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,8 @@ def create_test_yml(ctx, tool, run_tests, output, force, no_prompts):
@click.option("-a", "--all", is_flag=True, help="Run on all modules")
@click.option("--local", is_flag=True, help="Run additional lint tests for local modules")
@click.option("--passed", is_flag=True, help="Show passed tests")
def lint(ctx, tool, dir, key, all, local, passed):
@click.option("--fix", is_flag=True, help="Fix the module version if a newer version is available")
Copy link
Member

Choose a reason for hiding this comment

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

Here I'm not sure if --fix alone is a good flag name or it would be better to call it --fix_version. When running the nf-core modules lint with a --fix option, I would expect that this fixes potentially all the linting errors and not only the tool version. What do you think?

Copy link
Member

@ggabernet ggabernet May 31, 2022

Choose a reason for hiding this comment

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

Or maybe in a similar way that nf-core lint --fix works, by providing which kind of linting failure should be fixed. E.g. nf-core modules lint --fix versions. Not sure if it is possible to implement like this in this case

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's true, it can also be misleading with the --fix option from the pipeline linting. I will do some refactoring to --fix_version

Copy link
Member Author

Choose a reason for hiding this comment

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

I've checked the functionality of the --fix flag in the pipeline linting and I think it's different from what we have here with the version, as it allows the different lint tests, but the version fix is included in the linting of main.nf.
I changed the name of the flag to --fix-version, and we can implement the option to run specific lint tests in a different PR, do you agree?

def lint(ctx, tool, dir, key, all, local, passed, fix):
"""
Lint one or more modules in a directory.

Expand All @@ -607,7 +608,9 @@ def lint(ctx, tool, dir, key, all, local, passed):
try:
module_lint = nf_core.modules.ModuleLint(dir=dir)
module_lint.modules_repo = ctx.obj["modules_repo_obj"]
module_lint.lint(module=tool, key=key, all_modules=all, print_results=True, local=local, show_passed=passed)
module_lint.lint(
module=tool, key=key, all_modules=all, print_results=True, local=local, show_passed=passed, fix_version=fix
)
if len(module_lint.failed) > 0:
sys.exit(1)
except nf_core.modules.lint.ModuleLintException as e:
Expand Down
31 changes: 23 additions & 8 deletions nf_core/modules/lint/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,16 @@ def __init__(self, dir):
def _get_all_lint_tests():
return ["main_nf", "meta_yml", "module_todos", "module_deprecations"]

def lint(self, module=None, key=(), all_modules=False, print_results=True, show_passed=False, local=False):
def lint(
self,
module=None,
key=(),
all_modules=False,
print_results=True,
show_passed=False,
local=False,
fix_version=False,
):
"""
Lint all or one specific module

Expand All @@ -118,6 +127,7 @@ def lint(self, module=None, key=(), all_modules=False, print_results=True, show_
:param module: A specific module to lint
:param print_results: Whether to print the linting results
:param show_passed: Whether passed tests should be shown as well
:param fix_version: Update the module version if a newer version is available

:returns: A ModuleLint object containing information of
the passed, warned and failed tests
Expand Down Expand Up @@ -174,11 +184,11 @@ def lint(self, module=None, key=(), all_modules=False, print_results=True, show_

# Lint local modules
if local and len(local_modules) > 0:
self.lint_modules(local_modules, local=True)
self.lint_modules(local_modules, local=True, fix_version=fix_version)

# Lint nf-core modules
if len(nfcore_modules) > 0:
self.lint_modules(nfcore_modules, local=False)
self.lint_modules(nfcore_modules, local=False, fix_version=fix_version)

if print_results:
self._print_results(show_passed=show_passed)
Expand Down Expand Up @@ -282,19 +292,21 @@ def get_installed_modules(self):

return local_modules, nfcore_modules

def lint_modules(self, modules, local=False):
def lint_modules(self, modules, local=False, fix_version=False):
"""
Lint a list of modules

Args:
modules ([NFCoreModule]): A list of module objects
local (boolean): Whether the list consist of local or nf-core modules
fix_version (boolean): Fix the module version if a newer version is available
"""
progress_bar = rich.progress.Progress(
"[bold blue]{task.description}",
rich.progress.BarColumn(bar_width=None),
"[magenta]{task.completed} of {task.total}[reset] » [bold yellow]{task.fields[test_name]}",
transient=True,
console=console,
)
with progress_bar:
lint_progress = progress_bar.add_task(
Expand All @@ -305,9 +317,9 @@ def lint_modules(self, modules, local=False):

for mod in modules:
progress_bar.update(lint_progress, advance=1, test_name=mod.module_name)
self.lint_module(mod, local=local)
self.lint_module(mod, progress_bar, local=local, fix_version=fix_version)

def lint_module(self, mod, local=False):
def lint_module(self, mod, progress_bar, local=False, fix_version=False):
"""
Perform linting on one module

Expand All @@ -326,14 +338,17 @@ def lint_module(self, mod, local=False):

# Only check the main script in case of a local module
if local:
self.main_nf(mod)
self.main_nf(mod, fix_version, progress_bar)
self.passed += [LintResult(mod, *m) for m in mod.passed]
self.warned += [LintResult(mod, *m) for m in (mod.warned + mod.failed)]

# Otherwise run all the lint tests
else:
for test_name in self.lint_tests:
getattr(self, test_name)(mod)
if test_name == "main_nf":
getattr(self, test_name)(mod, fix_version, progress_bar)
else:
getattr(self, test_name)(mod)

self.passed += [LintResult(mod, *m) for m in mod.passed]
self.warned += [LintResult(mod, *m) for m in mod.warned]
Expand Down
115 changes: 105 additions & 10 deletions nf_core/modules/lint/main_nf.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,16 @@

import re
import nf_core
import logging
import requests

from galaxy.tool_util.deps.mulled.util import build_target
import nf_core.modules.module_utils

def main_nf(module_lint_object, module):
log = logging.getLogger(__name__)


def main_nf(module_lint_object, module, fix_version, progress_bar):
"""
Lint a ``main.nf`` module file

Expand Down Expand Up @@ -99,7 +106,7 @@ def main_nf(module_lint_object, module):
module.passed.append(("main_nf_script_outputs", "Process 'output' block found", module.main_nf))

# Check the process definitions
if check_process_section(module, process_lines):
if check_process_section(module, process_lines, fix_version, progress_bar):
module.passed.append(("main_nf_container", "Container versions match", module.main_nf))
else:
module.warned.append(("main_nf_container", "Container versions do not match", module.main_nf))
Expand Down Expand Up @@ -189,7 +196,7 @@ def check_when_section(self, lines):
self.passed.append(("when_condition", "when: condition is unchanged", self.main_nf))


def check_process_section(self, lines):
def check_process_section(self, lines, fix_version, progress_bar):
"""
Lint the section of a module between the process definition
and the 'input:' definition
Expand Down Expand Up @@ -235,14 +242,14 @@ def check_process_section(self, lines):
self.warned.append(("process_standard_label", "Process label unspecified", self.main_nf))

for l in lines:
if re.search("bioconda::", l):
if _container_type(l) == "bioconda":
bioconda_packages = [b for b in l.split() if "bioconda::" in b]
l = l.strip(" '\"")
if l.startswith("https://containers") or l.startswith("https://depot"):
if _container_type(l) == "singularity":
# e.g. "https://containers.biocontainers.pro/s3/SingImgsRepo/biocontainers/v1.2.0_cv1/biocontainers_v1.2.0_cv1.img' :" -> v1.2.0_cv1
# e.g. "https://depot.galaxyproject.org/singularity/fastqc:0.11.9--0' :" -> 0.11.9--0
singularity_tag = re.search(r"(?:/)?(?:biocontainers_)?(?::)?([A-Za-z\d\-_.]+?)(?:\.img)?['\"]", l).group(1)
if l.startswith("biocontainers/") or l.startswith("quay.io/"):
singularity_tag = re.search("(?:\/)?(?:biocontainers_)?(?::)?([A-Za-z\d\-_\.]+)(?:\.img)?['\"]", l).group(1)
if _container_type(l) == "docker":
# e.g. "quay.io/biocontainers/krona:2.7.1--pl526_5' }" -> 2.7.1--pl526_5
# e.g. "biocontainers/biocontainers:v1.2.0_cv1' }" -> v1.2.0_cv1
docker_tag = re.search(r"(?:[/])?(?::)?([A-Za-z\d\-_.]+)['\"]", l).group(1)
Expand Down Expand Up @@ -271,9 +278,30 @@ def check_process_section(self, lines):
last_ver = response.get("latest_version")
if last_ver is not None and last_ver != bioconda_version:
package, ver = bp.split("=", 1)
self.warned.append(
("bioconda_latest", f"Conda update: {package} `{ver}` -> `{last_ver}`", self.main_nf)
)
# If a new version is available and fix is True, update the version
if fix_version:
if _fix_module_version(self, bioconda_version, last_ver, singularity_tag, response):
progress_bar.print(f"[blue]INFO[/blue]\t Updating package '{package}' {ver} -> {last_ver}")
log.debug(f"Updating package {package} {ver} -> {last_ver}")
self.passed.append(
(
"bioconda_latest",
f"Conda package has been updated to the latest available: `{bp}`",
self.main_nf,
)
)
else:
progress_bar.print(
f"[blue]INFO[/blue]\t Tried to update package. Unable to update package '{package}' {ver} -> {last_ver}"
)
log.debug(f"Unable to update package {package} {ver} -> {last_ver}")
self.warned.append(
("bioconda_latest", f"Conda update: {package} `{ver}` -> `{last_ver}`", self.main_nf)
)
else:
self.warned.append(
("bioconda_latest", f"Conda update: {package} `{ver}` -> `{last_ver}`", self.main_nf)
)
else:
self.passed.append(("bioconda_latest", f"Conda package is the latest available: `{bp}`", self.main_nf))

Expand Down Expand Up @@ -340,3 +368,70 @@ def _is_empty(self, line):
if line.strip().replace(" ", "") == "":
empty = True
return empty


def _fix_module_version(self, current_version, latest_version, singularity_tag, response):
"""Updates the module version

Changes the bioconda current version by the latest version.
Obtains the latest build from bioconda response
Checks that the new URLs for docker and singularity with the tag [version]--[build] are valid
Changes the docker and singularity URLs
"""
# Get latest build
build = _get_build(response)

with open(self.main_nf, "r") as source:
lines = source.readlines()

# Check if the new version + build exist and replace
new_lines = []
for line in lines:
l = line.strip(" '\"")
build_type = _container_type(l)
if build_type == "bioconda":
new_lines.append(re.sub(rf"{current_version}", f"{latest_version}", line))
elif build_type == "singularity" or build_type == "docker":
# Check that the new url is valid
new_url = re.search(
"(?:')(.+)(?:')", re.sub(rf"{singularity_tag}", f"{latest_version}--{build}", line)
).group(1)
response_new_container = requests.get(
"https://" + new_url if not new_url.startswith("https://") else new_url, stream=True
)
log.debug(
f"Connected to URL: {'https://' + new_url if not new_url.startswith('https://') else new_url}, status_code: {response_new_container.status_code}"
)
if response_new_container.status_code != 200:
return False
new_lines.append(re.sub(rf"{singularity_tag}", f"{latest_version}--{build}", line))
else:
new_lines.append(line)

# Replace outdated versions by the latest one
with open(self.main_nf, "w") as source:
for line in new_lines:
source.write(line)

return True


def _get_build(response):
"""Get the latest build of the container version"""
build_times = []
latest_v = response.get("latest_version")
files = response.get("files")
for f in files:
if f.get("version") == latest_v:
build_times.append((f.get("upload_time"), f.get("attrs").get("build")))
return sorted(build_times, key=lambda tup: tup[0], reverse=True)[0][1]


def _container_type(line):
"""Returns the container type of a build."""
if re.search("bioconda::", line):
return "bioconda"
if line.startswith("https://containers") or line.startswith("https://depot"):
return "singularity"
if line.startswith("biocontainers/") or line.startswith("quay.io/"):
return "docker"