Update the cmd entry of the dvc.yaml file to add cmd as list option#1980
Conversation
Update the dvc.yaml documentation with the list possibility for the `cmd` entry.
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 |
jorgeorpinel
left a comment
There was a problem hiding this comment.
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
|
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). |
Do you think that the |
|
@ClementWalter I agree, that is not a desired behavior. List of commands could be only specified by hand by editing dvcfile, |
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 |
|
@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? |
| Running stage 'count': | ||
| $ python process.py numbers.txt > count.txt |
There was a problem hiding this comment.
This is the actual output? Including a $ character?
There was a problem hiding this comment.
yes, it has changed from the previous "with command: \t {command}" to this with line beginning with $
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Implemented this on treeverse/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.
There was a problem hiding this comment.
Let's discuss this on a different issue.
Created treeverse/dvc#5043 to discuss this further.
There was a problem hiding this comment.
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.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually I'm back to having an opinion 😆 — see treeverse/dvc#5043 (comment)
jorgeorpinel
left a comment
There was a problem hiding this comment.
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:
| $ 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 |
There was a problem hiding this comment.
I wonder if we need an example with multiple commands in repro...
skshetry
left a comment
There was a problem hiding this comment.
Rest looks good, just one issue with the output format.
|
I am not sure about the remaining changes to be brought to this PR. |
|
Hey no worries @ClementWalter I've assigned myself to finish up this one. Thanks! |
| Running stage 'count' with command: | ||
| python process.py numbers.txt > count.txt | ||
| Running stage 'count': |
There was a problem hiding this comment.
OK I removed "with command" from this file (for now, see treeverse/dvc#5043 (comment)) but there's another 4 instances/files with this text in the repo.
There was a problem hiding this comment.
I think this is mergeable pending #1980 (review) and #1980 (review) above, for a follow-up PR.
shcheklein
left a comment
There was a problem hiding this comment.
Thanks @ClementWalter , @jorgeorpinel , and @skshetry !
|
Looks like this is a 2.0 feature so I removed it from the |
* params: document --targets option * cmd: fux config file paths * Update content/docs/command-reference/params/diff.md Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com> * 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 <commits@restyled.io> * docs: misc copy edits * cmd: roll back wrong changes to repro Co-authored-by: Paweł Redzyński <pawelredzynski@gmail.com> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>
Update the dvc.yaml documentation with the list possibility for the
cmdentry.Core PR #4934
❗ 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. 🙏