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

Fix linting multiple remotes #2071

Merged
merged 15 commits into from
Dec 2, 2022

Conversation

mashehu
Copy link
Contributor

@mashehu mashehu commented Nov 29, 2022

Addresses #2052.

@mashehu mashehu requested a review from awgymer November 29, 2022 12:34
@awgymer
Copy link
Contributor

awgymer commented Nov 29, 2022

From the error logs:

LookupError: Could not find branch information for component
'custom/custom/dumpsoftwareversions'.Please remove the 'modules.json' and rerun
the command to recreate it

That path looks wrong, like it's derived custom as the org path?

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM! I also tested it and seems to work nicely 👌

@awgymer
Copy link
Contributor

awgymer commented Nov 29, 2022

There is another error because ModulesJson.get_all_components returns None if it would return {}.
There is only one place in the codebase that this is expected:

else:
components = self.modules_json.get_all_components(self.component_type).get(
self.modules_repo.remote_url
)
components = [
component if dir == "nf-core" else f"{dir}/{component}" for dir, component in components
]
if components is None:
raise UserWarning(
f"No {self.component_type[:-1]} installed from '{self.modules_repo.remote_url}'"
)

But actually if it ever returned None the list comprehension would error and if components is None would never be reached

This is wrong too. That actually uses .get so would fail! I don't think anywhere in the codebase expects or can handle a return value of None. (The list comp point still stands though - that will fail if .get returns None)

@awgymer
Copy link
Contributor

awgymer commented Nov 29, 2022

Error from test_modules_lint_patched_modules:

╭─ [!] 4 Module Test Warnings ─────────────────────────────────────────────────╮
│                                           ╷                ╷                 │
│ Module name                               │ File path      │ Test message    │
│╶──────────────────────────────────────────┼────────────────┼────────────────╴│
│ bismark/align                             │ modules/nf-co… │ Conda update:   │
│                                           │                │ bioconda::bism… │
│                                           │                │ 0.23.0 ->       │
│                                           │                │ 0.24.0          │
│ custom/dumpsoftwareversions               │ modules/nf-co… │ New version     │
│                                           │                │ available       │
│ fastqc                                    │ modules/nf-co… │ New version     │
│                                           │                │ available       │
│ custom/dumpsoftwareversions               │ modules/nf-co… │ Process name    │
│                                           │                │ not used for    │
│                                           │                │ versions.yml    │
│                                           ╵                ╵                 │
╰──────────────────────────────────────────────────────────────────────────────╯
╭─ [✗] 1 Module Test Failed ───────────────────────────────────────────────────╮
│                                           ╷                ╷                 │
│ Module name                               │ File path      │ Test message    │
│╶──────────────────────────────────────────┼────────────────┼────────────────╴│
│ bismark/align                             │ modules/nf-co… │ Local copy of   │
│                                           │                │ module does not │
│                                           │                │ match remote    │
│                                           ╵                ╵                 │
╰──────────────────────────────────────────────────────────────────────────────╯

And the difference:

input:                                                                    
 -    tuple val(meta), path(reads)                                              
 -    path index                                                                
 +    tuple val(meta), path(reads), path(index)      

@awgymer
Copy link
Contributor

awgymer commented Nov 29, 2022

test_modules_lint_no_gitlab fails because it sees modules:

custom/dumpsoftwareversions | /var/folders/q2/785gjdb95c952rl7m18rzmww0000gp/T/tmp7lru83qs/mypipeline/modules/nf-core/custom/dumpsoftwareversions
fastqc | /var/folders/q2/785gjdb95c952rl7m18rzmww0000gp/T/tmp7lru83qs/mypipeline/modules/nf-core/fastqc
multiqc | /var/folders/q2/785gjdb95c952rl7m18rzmww0000gp/T/tmp7lru83qs/mypipeline/modules/nf-core/multiqc

It seems that it is fetching all modules regardless of the org?
This makes sense since you are in self.pipeline_dir.
self.all_remote_modules is collated as a list of all modules from all remotes.

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #2071 (0a33a77) into dev (8fadd99) will increase coverage by 0.13%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##              dev    #2071      +/-   ##
==========================================
+ Coverage   67.84%   67.98%   +0.13%     
==========================================
  Files          43       43              
  Lines        5592     5597       +5     
==========================================
+ Hits         3794     3805      +11     
+ Misses       1798     1792       -6     
Impacted Files Coverage Δ
nf_core/components/info.py 80.00% <0.00%> (ø)
nf_core/components/remove.py 84.81% <100.00%> (+1.26%) ⬆️
nf_core/components/update.py 81.51% <100.00%> (ø)
nf_core/modules/lint/__init__.py 83.51% <100.00%> (+0.62%) ⬆️
nf_core/modules/lint/module_changes.py 87.50% <100.00%> (+1.78%) ⬆️
nf_core/modules/lint/module_version.py 82.60% <100.00%> (+0.79%) ⬆️
nf_core/modules/modules_repo.py 83.49% <100.00%> (ø)
nf_core/modules/nfcore_module.py 100.00% <100.00%> (ø)
nf_core/modules/lint/main_nf.py 66.15% <0.00%> (+1.52%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tests/modules/lint.py Outdated Show resolved Hide resolved
@mirpedrol mirpedrol merged commit da49046 into nf-core:dev Dec 2, 2022
@mashehu mashehu deleted the fix-linting-multiple-remotes branch December 2, 2022 11:49
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.

nf-core modules lint --all in a pipeline only works for a single remote
3 participants