Allow user to pass a list of commands for one single stage #4934

ClementWalter · 2020-11-21T18:27:36Z

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Fix #4373
Doc PR: treeverse/dvc.org#1980

dvc/stage/__init__.py

efiop · 2020-11-21T19:47:08Z

Thanks for the PR! 🙏 Note that your commit is not recognised by github as coming from your github profile. You need to add the email used in the commit to https://github.com/settings/emails . 🙂

efiop · 2020-11-22T04:08:30Z

Also, please don't forget to add the issue number or even "fixes #issuenumber" to the PR desc(or commit), so github tracks it automatically. #4373 🙂

ClementWalter · 2020-11-25T13:46:02Z

thanks for the feedback. Actually I was guessing it was hacky but, so easy to write down that I gave it a try. I'll work on this tonight

ClementWalter · 2020-11-25T17:58:14Z

This might be better but I am not sure about the loop and eventually I am not sure where to add tests for this change

dvc/stage/run.py

scripts/schema/dvc-yaml.json

efiop

Looks great! Just a few more comments.

dvc/stage/run.py

efiop

Looks good! We are lacking a simple func or unit test to make sure that we are actually able to run list of commands though. Could you add one, please? Ir could be a unit test for run_cmd or it could be a simple func test (adding a simple repo.repro test to tests/func/test_repro_multistage.py would be enough).

skshetry

Looks good overall. I have 2 questions regarding UI and multiline support.

skshetry · 2020-12-01T03:11:00Z

dvc/stage/run.py

The issue you linked to is also about multiline string.
Right now, it's not officially supported, but works depending on the shell.
What do you think of it? Should we support it?

I am not sure about your question: the issue mentions:

piping: this shout stay as is as stated by @efiop

&&: the purpose of this pr: officially allowing the user to define sequential commands in a single stage from its yaml definition.

I don't see much space to think about something else

@ClementWalter, as you are proposing to close the issue related to multiline/multiple commands, I just wanted to know what your opinion is on that, and if we should support it, as this list and the multiline string seems to be allowing the user the same thing.

I think I don't understand what you are trying to ask me

ok you mean @skshetry if all this work is worth doing because eventually it is only syntactic sugar for yaml?

ok I think I got it, just re-reading from the beginning. You mean: should we also support the parallel case, ie multi line string where every line should a command that runs in parallel?

@ClementwWalter I believe @skshetry was wondering about your opinion on whether or not we should treat multiline string as a list of commands when running it. #4373 (comment) I've mentioned that it should probably stay as it is, but there is an idea that maybe we should handle that better, similar as gha yaml does it. Let's keep that issue opened for that last question.

@efiop yes! actually re-reading this morning and re-reading yaml parsing rules it became 100% clear to me what was @skshetry's point, sorry about that yesterday. My guess is that we should not try to parse user input. Maybe we could add a flag in the stage definition about the desired behavior with a list of command: stop on fail or keep going?

skshetry · 2020-12-01T03:24:57Z

dvc/stage/run.py

Let's not add running on each command of the list as there's already a line for the whole stage.
Let's either print them all as a list before running or one-by-one:

Running stage "build": - echo hello - echo world

But I can see that the output gets messy if the script output gets mixed.
We could think of removing indent and add a ">" before the script:

Running stage "build": > echo hello hello > echo world world

I like the suggestion with >, accepted ;)

ClementWalter · 2020-12-02T18:47:16Z

Looks good! We are lacking a simple func or unit test to make sure that we are actually able to run list of commands though. Could you add one, please? Ir could be a unit test for run_cmd or it could be a simple func test (adding a simple repo.repro test to tests/func/test_repro_multistage.py would be enough).

Just added two tests @efiop tell me what do you think about them

efiop · 2020-12-02T19:39:36Z

dvc/stage/run.py

@pmrowla Could you please confirm that this will work as expected with checkpoints?

This should work as expected

tests/func/test_repro_multistage.py

ClementWalter · 2020-12-03T10:41:53Z

tests/func/test_repro_multistage.py

-    dump_yaml(PIPELINE_FILE, dvcfile_content)
-    dvc.reproduce(target="multi_commands")
-    assert (tmp_dir / "foo").read_text() == "First command\nSecond command\n"
+    (tmp_dir / "dvc.yaml").write_text(


cool, I was not sure if it was better to write yaml directly or to use the dump_yaml func indeed.

should the PIPELINE_FILE variable be used instead of direct dvc.yaml ?

Doesn't really matter in the test.

ClementWalter · 2020-12-03T10:43:25Z

tests/func/test_repro_multistage.py

+                - echo foo>foo
+                - echo bar>bar


much better because actually my version crashed on windows, thanks

efiop · 2020-12-03T10:49:45Z

Thank you @ClementWalter ! 🚀

ClementWalter · 2020-12-03T10:52:09Z

thanks you too! how is it that not all tests passes?

efiop · 2020-12-03T10:53:55Z

@ClementWalter Temporary issues with windows tests related to gitpython, please don't mind it, your tests have passed. 👍

efiop reviewed Nov 21, 2020

View reviewed changes

dvc/stage/__init__.py Outdated Show resolved Hide resolved

ClementWalter force-pushed the support_list_of_commands_in_yaml branch from ace930f to 6f3b432 Compare November 25, 2020 17:54

efiop reviewed Nov 27, 2020

View reviewed changes

dvc/stage/run.py Outdated Show resolved Hide resolved

efiop reviewed Nov 27, 2020

View reviewed changes

scripts/schema/dvc-yaml.json Outdated Show resolved Hide resolved

efiop reviewed Nov 27, 2020

View reviewed changes

jorgeorpinel reviewed Nov 29, 2020

View reviewed changes

dvc/stage/run.py Outdated Show resolved Hide resolved

ClementWalter mentioned this pull request Nov 30, 2020

Update the cmd entry of the dvc.yaml file to add cmd as list option treeverse/dvc.org#1980

Merged

efiop reviewed Nov 30, 2020

View reviewed changes

skshetry reviewed Dec 1, 2020

View reviewed changes

efiop reviewed Dec 2, 2020

View reviewed changes

tests/func/test_repro_multistage.py Outdated Show resolved Hide resolved

efiop reviewed Dec 2, 2020

View reviewed changes

tests/func/test_repro_multistage.py Outdated Show resolved Hide resolved

efiop reviewed Dec 2, 2020

View reviewed changes

tests/func/test_repro_multistage.py Outdated Show resolved Hide resolved

ClementWalter and others added 10 commits December 3, 2020 12:12

Allow list type in STAGE_DEFINITION scheme and join(cmd) if list

9bc97d5

Remove join in Stage.__init__ and update test

84fd314

Loop over commands in cmd_run

3df4daa

Improve logging of stage.run with detailed by command

12ac2b0

Fix test based on log message

1d9d8c6

Update command logging with >

3e77a80

Add tests for multi lines command

dfb9e5b

Update > to $ in stage command log to prevent confusion with > operator

4aed8b4

Fix PR review about test: single quote and read_text

8bb3279

tests: unify/clarify tests

a62f7cc

efiop force-pushed the support_list_of_commands_in_yaml branch from d947329 to a62f7cc Compare December 3, 2020 10:25

ClementWalter commented Dec 3, 2020

View reviewed changes

efiop merged commit 94f2a1d into treeverse:master Dec 3, 2020

skshetry mentioned this pull request Dec 7, 2020

run/repro: show command on dry-run, add ">" to script output #5041

Merged

2 tasks

efiop mentioned this pull request Dec 22, 2020

repro: add multi-cmd example treeverse/dvc.org#2045

Closed

Allow user to pass a list of commands for one single stage #4934

Allow user to pass a list of commands for one single stage #4934

Uh oh!

Conversation

ClementWalter commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

efiop commented Nov 21, 2020

Uh oh!

efiop commented Nov 22, 2020

Uh oh!

ClementWalter commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ClementWalter commented Nov 25, 2020

Uh oh!

Uh oh!

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

skshetry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClementWalter commented Dec 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop commented Dec 3, 2020

Uh oh!

ClementWalter commented Dec 3, 2020

Uh oh!

efiop commented Dec 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ClementWalter commented Nov 21, 2020 •

edited

Loading

ClementWalter commented Nov 25, 2020 •

edited

Loading