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

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

Merged
merged 21 commits into from
Dec 12, 2020

Conversation

ClementWalter
Copy link
Contributor

@ClementWalter ClementWalter commented Nov 29, 2020

Update the dvc.yaml documentation with the list possibility for the cmd entry.

Core PR #4934

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

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

Update the dvc.yaml documentation with the list possibility for the `cmd` entry.
@jorgeorpinel
Copy link
Contributor

Fix #4373

I don't think this docs PR fixes a core issue 🙂 please link to the core PR this matches instead (don't link them). Thanks

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Left a suggestion ^ Also:

  • The dvc.yaml example should show multiple commands, probably. Any other dvc.yaml examples throughout docs that could feature this?
  • And what about the cmd refs? At least https://dvc.org/doc/command-reference/run needs an update, I think. Maybe repro

Thanks

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Nov 29, 2020

Oh and BTW the PR checks are failing, please follow https://dvc.org/doc/user-guide/contributing/docs (we can also merge the restyled PR #1981 if needed but preferably, it shouldn't be necessary).

@ClementWalter
Copy link
Contributor Author

ClementWalter commented Nov 30, 2020

Do you think that the dvc run should handle a command, with, for instance, && so as to make it eventually a list in the dvc.yaml? I may be nice, but I am not sure it is the desired behavior because it requires some parsing of the user command, which, indeed, does not seem to be a preferred choice. @efiop, an idea about that?

@efiop
Copy link
Contributor

efiop commented Nov 30, 2020

@ClementWalter I agree, that is not a desired behavior. List of commands could be only specified by hand by editing dvcfile, dvc run doesn't need to support it.

@jorgeorpinel
Copy link
Contributor

dvc run doesn't need to support it.

Sounds good. But at the very least this will need checking the usage of the term "command" in repro (e.g. see -q option) and maybe other refs so it goes from singular to plural. Also it seems like the output of dvc repro will change slightly? There are a few examples that it would be ideal to update per this. Thanks

@ClementWalter
Copy link
Contributor Author

@jorgeorpinel I try to rephrase, hopefully it is clearer. I have added a use case for a list of commands, hopefully relevant as well. What do you think?

Comment on lines 256 to 257
Running stage 'count':
$ python process.py numbers.txt > count.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the actual output? Including a $ character?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it has changed from the previous "with command: \t {command}" to this with line beginning with $

Copy link
Contributor

Choose a reason for hiding this comment

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

So repro becomes some sort of background process that actually enters commands into the user's shell? Or we specifically print the $ character + command string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we print it. Indeed it went from previous repeated "running command", to "> {command}" suggested by @skshetry to "$ {command}" because the ">" were confusing with possible ">" inside a command (like echo > file)

Copy link
Member

Choose a reason for hiding this comment

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

@ClementWalter @skshetry @efiop this can create a mess across all docs where we use $ as a starting symbol for all shell commands. We'll need some mechanism to escape it, and even if we do we'll end up with confusing text anyway - mix of actual commands and outputs. Let's reconsider this please. Also out of curiosity how exactly is it related to the PR (multiple commands), I mean this particular change in UI/UX? what was the reason to change it?

Copy link
Member

Choose a reason for hiding this comment

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

Implemented this on iterative/dvc#5041.

not sure > is great since it can be part of the command

For now, we add > instead of $. Let's discuss this on a different issue.

Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss this on a different issue.

Created iterative/dvc#5043 to discuss this further.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, we add > instead of $.

Actually right now it's a tab, why change it if we're not sure? I'd vote to roll back that change and yes to the discussion.

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I'm back to having an opinion 😆 — see iterative/dvc#5043 (comment)

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Thanks for all the details! Let's try to summarize this last bit about the cmd field though. And no worries, I can finish the writing at some point but I do have some questions about it before I can do that. My suggestions for now:

content/docs/user-guide/dvc-files-and-directories.md Outdated Show resolved Hide resolved
content/docs/user-guide/dvc-files-and-directories.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel self-assigned this Dec 7, 2020
Comment on lines 254 to 257
$ dvc repro
Stage 'filter' didn't change, skipping
Running stage 'count' with command:
python process.py numbers.txt > count.txt
python process.py numbers.txt > count.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need an example with multiple commands in repro...

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved to #2045

Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

Rest looks good, just one issue with the output format.

content/docs/command-reference/repro.md Outdated Show resolved Hide resolved
@ClementWalter
Copy link
Contributor Author

I am not sure about the remaining changes to be brought to this PR.

@jorgeorpinel
Copy link
Contributor

Hey no worries @ClementWalter I've assigned myself to finish up this one. Thanks!

Comment on lines -247 to +252
Running stage 'count' with command:
python process.py numbers.txt > count.txt
Running stage 'count':
Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 12, 2020

Choose a reason for hiding this comment

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

OK I removed "with command" from this file (for now, see iterative/dvc#5043 (comment)) but there's another 4 instances/files with this text in the repo.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

I think this is mergeable pending #1980 (review) and #1980 (review) above, for a follow-up PR.

@shcheklein shcheklein merged commit 23247d8 into iterative:master Dec 12, 2020
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

@jorgeorpinel
Copy link
Contributor

Looks like this is a 2.0 feature so I removed it from the v1 branch in 365b768.

shcheklein pushed a commit that referenced this pull request Dec 24, 2020
* params: document --targets option

* cmd: fux config file paths

* Update content/docs/command-reference/params/diff.md

Co-authored-by: Jorge Orpinel <[email protected]>

* Revert "Update the cmd entry of the dvc.yaml file to add cmd as list option (#1980)"

This reverts commit 23247d8.

* Restyled by prettier (#2042)

Co-authored-by: Restyled.io <[email protected]>

* docs: misc copy edits

* cmd: roll back wrong changes to repro

Co-authored-by: Paweł Redzyński <[email protected]>
Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>
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.

5 participants