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

Better support pathogen repositories which place workflows in subdirectories #355

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented Jan 31, 2024

…as this is the direction we're moving.

The main change is making the filesystem isolation boundary (i.e. what's mapped to /nextstrain/build in a container) a separate thing from the workflow and initial working directory (i.e. what's given to nextstrain build). In this codebase, these two things are referred to as the build volume (aka opts.build) and the working volume. Historically, the working volume given to the runners for nextstrain build and nextstrain shell was the build volume; now, they're separately considered and sometimes differ.

See the included changelog entry for usage details and rationale. For background, I made the initial proposal for this feature¹ on a PR in our pathogen-repo-template repository and some discussion ensued.

¹ nextstrain/pathogen-repo-guide#16 (comment)
or https://github.com/tsibley/blab-standup/blob/master/2024-01-25.md

Related issue(s)

Based on #354.

Checklist

  • Checks pass

@tsibley
Copy link
Member Author

tsibley commented Jan 31, 2024

Some new tests are failing on Windows, due to path handling.

Expected:
    NamedVolume(name='build/ingest', src=...Path('.../tests/data/pathogen-repo/ingest'), dir=True, writable=True)
Got:
    NamedVolume(name='build\\ingest', src=WindowsPath('D:/a/cli/cli/tests/data/pathogen-repo/ingest'), dir=True, writable=True)

I'll fix it up.

…ctories

…as this is the direction we're moving.

The main change is making the filesystem isolation boundary (i.e. what's
mapped to `/nextstrain/build` in a container) a _separate thing_ from
the workflow and initial working directory (i.e. what's given to
`nextstrain build`).  In this codebase, these two things are referred to
as the _build volume_ (aka `opts.build`) and the _working volume_.
Historically, the working volume given to the runners for `nextstrain
build` and `nextstrain shell` _was_ the build volume; now, they're
separately considered and sometimes differ.

See the included changelog entry for usage details and rationale.  For
background, I made the initial proposal for this feature¹ on a PR in our
pathogen-repo-template repository and some discussion ensued.

¹ <nextstrain/pathogen-repo-guide#16 (comment)>
  or <https://github.com/tsibley/blab-standup/blob/master/2024-01-25.md>
@tsibley tsibley force-pushed the trs/build/workflows-in-subdirs branch from bf858d2 to fc2afdf Compare January 31, 2024 23:44
Base automatically changed from trs/build/error-if-volumes-unsupported to master February 1, 2024 20:36
Copy link
Contributor

@huddlej huddlej left a comment

Choose a reason for hiding this comment

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

I don't have strong feelings about the technical implementation, but I love the idea of the top-level indicator file that works akin to .git to designate a pathogen directory. I also love that this approach allows us to run the Nextstrain CLI the way that feels most natural (to me, at least) by specifying the directory with the workflow we want to run (nextstrain build ingest) and not needing to also specify the path to the Snakefile (nextstrain build . -s ingest/Snakefile which reveals implementation details that we don't necessarily want to formalize).

I also love that you made this happen so quickly after our random coffee/lab chat about it, @tsibley, even if it was to head-off my terrible suggestions involving symlinks! 🌈

joverlee521 added a commit to nextstrain/pathogen-repo-guide that referenced this pull request Feb 2, 2024
This is currently an empty file to indicate the top level pathogen repo.
The inclusion of this file allows the `nextstrain build` invocation to
work from any directory regardless of runtime.

See nextstrain/cli#355 for more details.
@tsibley tsibley merged commit eaf060e into master Feb 5, 2024
37 checks passed
@tsibley tsibley deleted the trs/build/workflows-in-subdirs branch February 5, 2024 22:26
joverlee521 added a commit to nextstrain/pathogen-repo-guide that referenced this pull request Feb 8, 2024
This is currently an empty file to indicate the top level pathogen repo.
The inclusion of this file allows the `nextstrain build` invocation to
work from any directory regardless of runtime.

See nextstrain/cli#355 for more details.
j23414 added a commit to nextstrain/nipah that referenced this pull request Aug 2, 2024
This is currently an empty file to indicate the top level pathogen repo.
The inclusion of this file allows the `nextstrain build` invocation to
work from any directory regardless of runtime.

See nextstrain/cli#355 for more details.
j23414 added a commit to nextstrain/norovirus that referenced this pull request Oct 25, 2024
This is currently an empty file to indicate the top level pathogen repo.
The inclusion of this file allows the `nextstrain build` invocation to
work from any directory regardless of runtime.

See nextstrain/cli#355 for more details.
j23414 added a commit to nextstrain/norovirus that referenced this pull request Oct 26, 2024
This is currently an empty file to indicate the top level pathogen repo.
The inclusion of this file allows the `nextstrain build` invocation to
work from any directory regardless of runtime.

See nextstrain/cli#355 for more details.
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.

2 participants