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

Allow pathogen-repo-ci to run workflow in subdir via build-dir arg #62

Closed
wants to merge 1 commit into from

Conversation

corneliusroemer
Copy link
Member

@corneliusroemer corneliusroemer commented Sep 25, 2023

Description of proposed changes

I'm trialling out putting our main auspice-producing workflow in a folder of a pathogen repo (this is what's currently suggested by our pathogen-repo-template).

This is currently not supported by pathogen-repo-ci as there's an implicit assumption that the root of the workflow is in the root of the repo.

In this PR I add support for specifying the workflow directory through an argument passed to pathogen-repo-ci.

Changes are backwards compatible as the default is set to what we previously used: .

Prior attempt: #57 (comment) this had a bug and so I reverted it.

Now the bug has been addressed. See test run: https://github.com/nextstrain/monkeypox/actions/runs/6330482320/job/17192978189#step:9:17

With the way the argument is used, the build-dir argument must not end in trailing slash. This is mentioned in the documentation of the argument.

@corneliusroemer

This comment was marked as outdated.

@corneliusroemer corneliusroemer removed the request for review from tsibley September 25, 2023 22:19
@corneliusroemer corneliusroemer marked this pull request as draft September 27, 2023 17:37
@corneliusroemer corneliusroemer requested review from tsibley, victorlin and a team September 27, 2023 19:32
@corneliusroemer corneliusroemer marked this pull request as ready for review September 27, 2023 19:32
@corneliusroemer corneliusroemer changed the title Fix interpolation of configurable build dir Allow pathogen-repo-ci to run workflow in subdir via build-dir arg Sep 27, 2023
.github/workflows/pathogen-repo-ci.yaml Outdated Show resolved Hide resolved
.github/workflows/pathogen-repo-ci.yaml Outdated Show resolved Hide resolved
@victorlin victorlin linked an issue Oct 9, 2023 that may be closed by this pull request
1 task
@joverlee521 joverlee521 self-assigned this Oct 18, 2023
@joverlee521
Copy link
Contributor

I think the updates to the pathogen-repo-ci here is really grabbing for the flexibility that is allowed in pathogen-repo-build.

The only crucial step that is missing from pathogen-repo-build is the copying of example data when available.
We could add this step to individual pathogen repos by including Snakemake rules to copy over the example data as part of custom profiles. This is currently implemented in the seasonal-flu repo's ci profile and gives the advantage of allowing users to run the example build without having to manually copy the data over.

If we go with this pattern, we can slowly phase out the pathogen-repo-ci workflow and just maintain a single pathogen-repo-build workflow.

@joverlee521
Copy link
Contributor

I force-pushed updates to this PR and implemented the alternative approach of using pathogen-repo-build in monkeypox so we can make direct comparisons of the two approaches.

The pathogen-repo-ci takes care of more things behind the scenes so the workflow call is definitely much more succinct than the pathogen-repo-build workflow call. Is that enough to justify continuing to update/maintain both workflows?

@tsibley
Copy link
Member

tsibley commented Oct 19, 2023

The pathogen-repo-ci takes care of more things behind the scenes so the workflow call is definitely much more succinct than the pathogen-repo-build workflow call. Is that enough to justify continuing to update/maintain both workflows?

One of the mismatches in the intents of pathogen-repo-ci and pathogen-repo-build is that the former expects to do more testing of runtimes while the latter does not. And that's what accounts for the verbose call to the latter when using it as a substitute for the former, e.g. needing a runtime matrix and threading that thru.

It's been nice in the past with pathogen-repo-ci to be able to update it to cover/test more bits (e.g. extending it to cover the Conda runtime too) without having to go update every call to it. Switching to pathogen-repo-build would lose that ability.

What if we keep pathogen-repo-ci but internally have it call pathogen-repo-build? Is that tractable?

@joverlee521
Copy link
Contributor

What if we keep pathogen-repo-ci but internally have it call pathogen-repo-build? Is that tractable?

This seems reasonable. We would just have to thread the inputs through to the pathogen-repo-build.
Nesting reusable workflows is possible, I just haven't tried it yet. Will look into it.

@joverlee521 joverlee521 mentioned this pull request Oct 20, 2023
1 task
@joverlee521
Copy link
Contributor

Nesting reusable workflows is possible, I just haven't tried it yet. Will look into it.

Tried this out in #65

@tsibley
Copy link
Member

tsibley commented Oct 23, 2023

Tried this out in #65 … [It's] doable and it works, but I'm not a big fan of the downsides of this approach.

Hmm. I see what you mean. Feels a bit like six of one, half a dozen of the other.

We could add this step to individual pathogen repos by including Snakemake rules to copy over the example data as part of custom profiles. This is currently implemented in the seasonal-flu repo's ci profile and gives the advantage of allowing users to run the example build without having to manually copy the data over.

So maybe adopting your suggestion of this convention of a CI profile (at a typical path) + switching from pathogen-repo-cipathogen-repo-build is the way to go?

With the development of the pathogen-repo-template¹, we are reorganizing
pathogen repositories to move workflows into their own directories.
This change in monkeypox² revealed that the pathogen-repo-ci workflow no
longer works with this new file organization because it expects
`nextstrain build` to run with the root of the pathogen repository.

This commit adds support for this change by allowing users to define
the directory to use for the `nextstrain build` command.

Changes are backwards compatible as the default is set to `.` to point
to the root of the pathogen repository.

¹ https://github.com/nextstrain/pathogen-repo-template
² nextstrain/mpox#198

Co-authored-by: Cornelius Roemer <[email protected]>
@joverlee521 joverlee521 force-pushed the configurable-build-dir branch from 3715982 to d340a70 Compare November 7, 2023 01:33
@joverlee521
Copy link
Contributor

So maybe adopting your suggestion of this convention of a CI profile (at a typical path) + switching from pathogen-repo-ci → pathogen-repo-build is the way to go?

This made sense when I was thinking in the context of individual pathogen repo CIs. However, maybe not as nice for the CI jobs in docker-base/conda-base/(maybe augur?) if we have to add on to the matrix of inputs.

We would still have to deal with the additional permissions for pathogen-repo-build if we ever tackle the docker.io vs github.io registry issue for docker-base. We would also have to deal with additional permissions for pathogen-repo-ci if we ever update it to use the workflow-context action. I also feel like things are up in the air because of the waffling on the standard file organization...

Instead of making sweeping changes when we are still unsure about things, I'm fine with pausing on this PR. I'm going to just fix the mpox repo CI with the pathogen-repo-build workflow without affecting other repos.

tsibley added a commit that referenced this pull request Jan 30, 2024
Fixes failures due to the recent reorganization of the zika repo to
match our proposed common layout going forward, which pathogen-repo-ci
doesn't yet support.¹

¹ <#62>
@joverlee521
Copy link
Contributor

This made sense when I was thinking in the context of individual pathogen repo CIs. However, maybe not as nice for the CI jobs in docker-base/conda-base/(maybe augur?) if we have to add on to the matrix of inputs.

Looping back to this after ~6 months: noodling on the idea to use pathogen-repo-build within pathogen CI workflows and add the workflow_call trigger to each CI workflow. Then the docker-base/conda-base CI workflows can just call the individual repo CI workflows.

The one issue with this idea is that the uses call cannot be dynamic, so we wouldn't be able to directly use a matrix of repo names. There's a workaround, but not sure if it's worth pursuing...

@genehack
Copy link
Contributor

Based on our lab meeting conversation today, I'm going to close this issue out in favor of a new one that I will be writing shortly.

@genehack genehack closed this May 24, 2024
@victorlin victorlin deleted the configurable-build-dir branch June 6, 2024 19:34
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.

ENH: Allow pathogen-repo-ci to invoke workflow in subfolder
5 participants