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.

Sign up for GitHub

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

Jump to bottom

Update odgi to version 0.9.0 #7313

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Update odgi to version 0.9.0 #7313

wants to merge 6 commits into from
+70 −60

Conversation

heuermh
Copy link
Contributor

@heuermh heuermh commented Jan 15, 2025 •
edited
Loading

Fixes nf-core/pangenome#214

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

@SPPearce
Copy link
Contributor

Could you please update ogdi/build and ogdi/layout to actually check something in the output? Currently their tests only check for success. Checking for file existence and the versions channel is fine.
The linting would normally fail because there is no check on the versions in the snapshot, but here there is a snapshot, it just isn't being used.

@heuermh heuermh requested a review from subwaystation January 17, 2025 18:29
@subwaystation
Copy link
Contributor

subwaystation commented Jan 20, 2025 •
edited
Loading

odgi build creates a binary file. You can try to run odgi stats and compare with expected numbers. Not sure what else to do here.
odgi layout is not deterministic, so I am not sure how to test the output here. It's a visual thing. U can count the number of node coordinates in an optional generated TSV, but that's all I can think of.

subwaystation
subwaystation reviewed Jan 20, 2025
View reviewed changes
modules/nf-core/odgi/draw/tests/main.nf.test.snap
"nf-test": "0.9.2",
"nextflow": "24.10.3"
},
"timestamp": "2025-01-15T14:46:28.331207"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing EOL

subwaystation
subwaystation reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@subwaystation subwaystation left a comment

Choose a reason for hiding this comment

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

There are some EOLs missing, but mostly in auto-generated files, so not a must do I think.

I understand @SPPearce concerns about odgi build and odgi layout, but in practice it is not easy to either parse a binary file or to test the output of a non-deterministic tool.

modules/nf-core/odgi/sort/tests/main.nf.test.snap
},
"timestamp": "2024-08-13T09:40:56.821872"
"timestamp": "2025-01-15T14:47:49.015053"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing EOL

modules/nf-core/odgi/squeeze/tests/main.nf.test.snap
},
"timestamp": "2024-08-13T10:09:07.409463"
"timestamp": "2025-01-15T14:48:22.366465"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing EOL

modules/nf-core/odgi/stats/tests/main.nf.test.snap
},
"timestamp": "2024-08-09T09:55:15.438306"
"timestamp": "2025-01-15T14:48:52.45995"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing EOL

modules/nf-core/odgi/unchop/environment.yml
@@ -2,4 +2,4 @@ channels:
- conda-forge
- bioconda
dependencies:
- bioconda::odgi=0.8.4
Copy link
Contributor

Choose a reason for hiding this comment

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

missing EOL

modules/nf-core/odgi/unchop/tests/main.nf.test.snap
},
"timestamp": "2024-08-13T09:38:53.743369"
"timestamp": "2025-01-15T14:49:21.848525"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing EOL

modules/nf-core/odgi/view/tests/main.nf.test.snap
"nf-test": "0.9.2",
"nextflow": "24.10.3"
},
"timestamp": "2025-01-15T14:49:51.203182"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the EOL is not relevant in snaps?

modules/nf-core/odgi/viz/tests/main.nf.test.snap
"nf-test": "0.9.2",
"nextflow": "24.10.3"
},
"timestamp": "2025-01-15T14:50:21.102589"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

cause EOcmake -H. -Bbuild && cmake --build build -- -j 3L is everywhere in each snap

@SPPearce
Copy link
Contributor

There are some EOLs missing, but mostly in auto-generated files, so not a must do I think.

I understand @SPPearce concerns about odgi build and odgi layout, but in practice it is not easy to either parse a binary file or to test the output of a non-deterministic tool.

You can test the existance of the output file though, which the current test was not doing. You should at least test the contents of the versions.yml file.

@subwaystation
Copy link
Contributor

There are some EOLs missing, but mostly in auto-generated files, so not a must do I think.
I understand @SPPearce concerns about odgi build and odgi layout, but in practice it is not easy to either parse a binary file or to test the output of a non-deterministic tool.

You can test the existance of the output file though, which the current test was not doing. You should at least test the contents of the versions.yml file.

Ah sorry, I missed this!

@subwaystation
Copy link
Contributor

We need a versions.yml test for both build and layout @heuermh, then, right? You already check for file existence, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@subwaystation subwaystation subwaystation left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update odgi version in modules to 0.9.0
3 participants
@heuermh @SPPearce @subwaystation