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

Honour always tag even when step selection is in use. #272

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

JSKenyon
Copy link
Collaborator

@JSKenyon JSKenyon commented Apr 9, 2024

Currently, invoking stimela run recipe.yml -s dependent_step will fail if dependent_step depends on the outputs of a preceding step. The user can specify stimela run recipe.yml -s dependency_step -s dependent_step, but this can become tedious and may require more in-depth recipe knowledge. The natural mechanism for handling this case is the always tag i.e. steps tagged as always are integral to recipe functionality and as such should always be run. Currently, -s will completely ignore any tags. This PR instead augments the selected steps with the always steps. One downside of this approach is that the always steps may be run even when they are unnecessary i.e. if a users only runs steps prior to the always step, they cannot be a requirement. @o-smirnov thoughts?

@JSKenyon
Copy link
Collaborator Author

JSKenyon commented Apr 9, 2024

My feeling is that always should be reserved for steps which can be run independently of any steps outside the always group. An example of this would be the ms-info step of the parrot pipeline. It should always be run as it is necessary for other steps. The alternative is to support a step_requires field but that may become quite involved.

@JSKenyon JSKenyon requested a review from o-smirnov April 9, 2024 13:14
@o-smirnov
Copy link
Member

o-smirnov commented Apr 9, 2024

Thanks @JSKenyon I like that idea. Do we need an extra command-line option to explicitly skip (--skip/-k) a step tagged with always?

P.S. I've already been thinking about auto-dependencies for a future version: #254 (comment)

@JSKenyon
Copy link
Collaborator Author

JSKenyon commented Apr 9, 2024

Thanks @JSKenyon I like that idea. Do we need an extra command-line option to explicitly skip (--skip/-k) a step tagged with always?

I have added a simple (--skip-step/-k) command for explicitly disabling specific steps. This should also work when you want to skip a step inside a tagged group.

Copy link
Member

@o-smirnov o-smirnov left a comment

Choose a reason for hiding this comment

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

Should -k/--skip also not apply inside a range selected with -s FROM:TO?

stimela/commands/run.py Outdated Show resolved Hide resolved
@JSKenyon
Copy link
Collaborator Author

I have decided to take a stab at rewriting the restrict_steps function. Will resubmit for review when I am done.

@JSKenyon JSKenyon requested a review from o-smirnov April 10, 2024 11:54
@JSKenyon
Copy link
Collaborator Author

@o-smirnov I have rewritten some of the restrict logic and I believe that it is now a little clearer. This probably results in slight changes in behaviour but I think the new approach is more powerful than the old. Specifically, -t and -s can now be combined e.g. stimela run recipe.yml -t some_steps -s extra_step. This will run all the tagged steps in addition to the extra step. Steps tagged as always will always run unless explicitly skipped with -k. -k now supports the same behaviour as -s for ranges of steps. Let me know what you think.

Copy link
Member

@o-smirnov o-smirnov left a comment

Choose a reason for hiding this comment

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

Looks good and is extremely useful, thanks @JSKenyon!

@o-smirnov o-smirnov merged commit aed211e into master Apr 16, 2024
3 checks passed
@o-smirnov o-smirnov deleted the honour-always branch April 16, 2024 13:08
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