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

[GEN-1653] Add integration tests to GH actions workflow #582

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

rxu17
Copy link
Contributor

@rxu17 rxu17 commented Jan 9, 2025

Purpose: This is a draft PR. This adds automated integration tests for each step of the genie pipeline into our GH actions workflow. Still need to do the following:

  • add conditions for when this should occur so it doesn't run every time there is a code change.
  • build-container step should be pre-req to this step

Testing:
Ran all testing pipeline steps here: https://github.com/Sage-Bionetworks/Genie/actions/runs/12662104568. It took close to an hour (mainly validation and consortium release steps taking the longest). Still need to look into why the case (separate ticket)

  • Verify pipeline steps don't have any underlying data changes with comparison script

@rxu17 rxu17 closed this Jan 22, 2025
@rxu17 rxu17 reopened this Jan 22, 2025
@rxu17 rxu17 marked this pull request as ready for review January 23, 2025 02:06
@rxu17 rxu17 requested a review from a team as a code owner January 23, 2025 02:06
@rxu17 rxu17 requested review from danlu1 and thomasyu888 January 23, 2025 02:39
docker run -d --name genie-container \
-e SYNAPSE_AUTH_TOKEN="${{ secrets.SYNAPSE_AUTH_TOKEN }}" \
ghcr.io/sage-bionetworks/genie:${{ env.BRANCH_NAME }} \
sh -c "while true; do sleep 1; done"
Copy link
Member

@thomasyu888 thomasyu888 Jan 24, 2025

Choose a reason for hiding this comment

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

whats the motivation of this while true statement?

Whats your rationale for starting the container instead of just having 5 docker run commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm from my understanding I thought that we had to keep the container "open" while running the code to keep it all in one container run/same state persistence. However thinking about it now, it should probably mirror nextflow where each step is probably separate docker run.

I also wonder if this is the reason why I'm seeing differences in the outputs compared to outputs when I run the nextflow pipeline (something I am investigating ...)

Copy link
Member

Choose a reason for hiding this comment

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

Differences in outputs?! What are the differences?

Copy link
Contributor Author

@rxu17 rxu17 Jan 24, 2025

Choose a reason for hiding this comment

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

All in the clinical consortium files. It's dropping more records for some reason (at least more compared to the nextflow pipeline run)

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 Great work here, I'll let Dan be the final reviewer but minor comments

- '**.md' # All Markdown files
- '**/docs/**' # Documentation directory
- '.github/workflows/codeql.yml' # Code scanner
- '.github/workflows/build_docs.yml' # mkdocs GH workflow
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some paths I think we can ignore as well: tests/testthat, and .pre-commit-config.yaml. There maybe more. Since it's mainly about running through the pipeline, do you think we can use path instead to only point to related scripts folder?

run: |
docker pull ghcr.io/sage-bionetworks/genie:${{ env.BRANCH_NAME }}

- name: Start Docker Container
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

3 participants