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

pathogen-repo-ci: Run with --ambient in addition to --docker #25

Closed
victorlin opened this issue Jul 1, 2022 · 8 comments
Closed

pathogen-repo-ci: Run with --ambient in addition to --docker #25

victorlin opened this issue Jul 1, 2022 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@victorlin
Copy link
Member

Currently, pathogen-repo-ci runs with Docker only:

- run: nextstrain build --docker . ${{ inputs.build-args }}

It would be good to also run with --native to catch issues such as nextstrain/ncov#968.

@tsibley
Copy link
Member

tsibley commented Jul 5, 2022

Definitely understand this desire, though it will ~double CI run times for all pathogen repos and significantly increase complexity of the shared workflow. It would probably be better to make a separate shared workflow and invoke it separately (this will allow opt-in and also parallel jobs).

@victorlin
Copy link
Member Author

For parallel running: instead of a separate workflow, what about a separate job within this workflow? There can be boolean inputs used to condition execution of jobs (e.g. run_docker, run_native). Allowing opt-in would be achieved by setting run_native's default to false.

@victorlin victorlin moved this from New to Prioritized in Nextstrain planning (archived) Jul 12, 2022
@tsibley
Copy link
Member

tsibley commented Jul 12, 2022

Ah, sure. I guess instead of even a separate job within this workflow, it could be one job with a matrix of conditions to help reduce repetition.

@victorlin victorlin self-assigned this Jul 12, 2022
@tsibley
Copy link
Member

tsibley commented Aug 22, 2023

We still don't test our pathogen repos with the ambient (née native) runtime… but I did expand pathogen-repo-ci to support testing on multiple runtimes and changed the default to Docker + Conda instead of just Docker. This was to support invoking pathogen-repo-ci for several repos as a smoke test for new Docker runtime images and Conda runtime packages.

The upshot though is it would much easier and clearer now to add testing of the ambient runtime too, though I'm still not sure we want to commit to always doing so for all pathogen repos.

@tsibley tsibley changed the title pathogen-repo-ci: Run with --native in addition to --docker pathogen-repo-ci: Run with --ambient in addition to --docker Aug 22, 2023
@corneliusroemer
Copy link
Member

I'm not sure there's any need to test "ambient" as "ambient" could be anything. It would only make sense if we specify an environment.yaml as alternative way to run compared to conda and `docker.

I'm not aware of many such environment.yamls existing. Would not be a bad idea, but that's probably the first step.

@tsibley
Copy link
Member

tsibley commented Sep 26, 2023

Ah, the conception of this issue is not to test every possible ambient environment. It's to test that workflows aren't too tightly-coupled to one specific runtime, e.g. Docker, and to avoid bugs like the one noted in the issue description above.

Note that this issue predates the Conda runtime's existence; Docker or ambient were what folks used. And it predates the multi-runtime testing that now occurs regularly (see my last comment above).

With routine multi-runtime testing nowadays, the usefulness of testing an ambient environment is lower: it'd be mostly to check that our documented/suggested/example ambient setup works as expected and not as a guard against too-tight runtime-workflow coupling as this issue was originally conceived.

I'm not aware of many such environment.yamls existing. Would not be a bad idea, but that's probably the first step.

I don't think we want to return to publishing a Conda environment file; there's history there.

@victorlin
Copy link
Member Author

it'd be mostly to check that our documented/suggested/example ambient setup works as expected and not as a guard against too-tight runtime-workflow coupling as this issue was originally conceived.

This be done quite easily with a mamba install, but in practice, users' ambient setups will differ greatly, even if just due to not upgrading to newer versions over time. I don't see much value in running pathogen-repo-ci with --ambient and would opt to close this as "not planned".

@tsibley
Copy link
Member

tsibley commented Sep 26, 2023

Yeah, I'm ok with "not planned". I think the value of doing it now is significantly less than it was when originally proposed.

@tsibley tsibley closed this as not planned Won't fix, can't repro, duplicate, stale Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Prioritized
Development

No branches or pull requests

3 participants