Fix steps tagged with never being ignored even when selected by other tags.

[JSKenyon]

This is a fix for a minor bug I introduced. Sorry @o-smirnov! Current behaviour will not run steps tagged with never if they are selected by tag i.e. the following:

  steps:
    flag-save:
      info: |
        Create a backup of the original flags on the dataset. This step is only
        run when the init tag is selected.
      tags: [init, never]
      cab: casa.flagman
      params:
        ms: =recipe.ms
        mode: save
        versionname: =recipe.initial-flag-version

will not be run by stimela run recipe.yaml -t init. I don't think this is what we want. This change will disregard never tags on steps picked by -s or -t i.e. never is only honoured when the entire recipe is run.

[JSKenyon]

I am open to other suggestions - technically, we could also this to -t only i.e. we could make it so that -s step: would still honour never.

[o-smirnov]

I think -s FROM:TO should honour never, but -s STEP should override it. This would be consistent with how it is (or was) for the interplay between -s and the explicit skip: True attribute, which gets overridden when a single step is cherry-picked.

This does mean that -s has two subtly different meanings in single-step and range mode. Do you think that's potentially confusing?

[o-smirnov]

Another question (will look in the code myself later when I've got time) -- is the -s SUBSTEP.FROM:TO syntax preserved? And should we support a -t SUBSTEP.TAG option? @Athanaseus did find this fine-grained control of steps within subrecipes (particularly, for-loop subrecipes) useful.

[JSKenyon]

Ah, very good question. I didn't have an example to hand to test that out. I did try to modify the substep code consistently though. I haven't set up a sub-recipe myself, so if you have an example lying around, that would be great.

[JSKenyon]

I think -s FROM:TO should honour never, but -s STEP should override it. This would be consistent with how it is (or was) for the interplay between -s and the explicit skip: True attribute, which gets overridden when a single step is cherry-picked.

This does mean that -s has two subtly different meanings in single-step and range mode. Do you think that's potentially confusing?

I also thought about that. I don't think it is too confusing provided we document it. -s step means that a step will be run, ignoring all tags/skip behaviour (except skip_if_output I believe). The alternative would be to split out the functionality a little differently e.g:

• -s: selects steps or ranges of steps, honouring all tags/skips.
• -cp selects steps or ranges of steps, ignoring all tags/skips (cherry-picking).
• deprecate -e in favour of -cp.

[o-smirnov]

The alternative would be to split out the functionality a little differently e.g:

Having thought about it, my preference is to keep the -s STEP semantics. Extra options bring their own confusion. If you're truly cherry-picking one step, presumably you really want to run it come hell or high water.

[JSKenyon]

Having thought about it, my preference is to keep the -s STEP semantics. Extra options bring their own confusion. If you're truly cherry-picking one step, presumably you really want to run it come hell or high water.

Ok so in summary we want to:

• Honour never when used with a range of steps e.g. -s step_x:step_y.
• Ignore never when a single step is specified e.g. -s step.
• Ignore never when the step is selected via a tag e.g. -t init (unless you disagree @o-smirnov).

The fix for the above is trivial, just want to be sure I am doing the right thing.

[o-smirnov]

Confirmed. This also matches the ansible never-always tag behaviour, so we're in good company.

[JSKenyon]

Ok, done. @o-smirnov could I trouble you to test the subrecipe case when you have a chance? I don't mind putting in more work but a representative recipe + usage example would be nice.

[JSKenyon]

@o-smirnov I have updated the PR with two extra asserts which should catch bogus tags.

[o-smirnov]

Thanks @JSKenyon. I've added a fix to propagate step and tag selection into subrecipes. There's a test recipe checked in under stimela_tests which lets you play with this functionality, e.g.:

$ stimela run test_subrecipes.yml -d -t foo -k s4 -t s3.foo 

The -d/--dry-run option is useful, as it just shows you the step selection without running anything.

If you feel motivated, you can add a proper test for this with console output checking. I'm off to assemble a TART so can't do it just now.

@Athanaseus @SpheMakh please re-review this so we can merge it.

[Athanaseus]

lgtm

[JSKenyon]

Sorry @Athanaseus - I changed the logging slightly. Could you please review and approve again? No functional changes.

[Athanaseus]

nice