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

stage:add: decide on -c command vs remainder arg for user's script #5342

Closed
Tracked by #5367
skshetry opened this issue Jan 27, 2021 · 7 comments · Fixed by #5350
Closed
Tracked by #5367

stage:add: decide on -c command vs remainder arg for user's script #5342

skshetry opened this issue Jan 27, 2021 · 7 comments · Fixed by #5350
Assignees
Labels
discussion requires active participation to reach a conclusion enhancement Enhances DVC p1-important Important, aka current backlog of things to do

Comments

@skshetry
Copy link
Member

skshetry commented Jan 27, 2021

On dvc run, the remainder positional argument is considered as a script.

$ dvc run -n stage_name -o bar -w data echo bar > bar

But, we have heard complaints from users that it's confusing, as trying to add certain flags to dvc run at the end is now assumed to be part of the user's script.

So, on stage:add, we moved it under the -c/--command flag and now needs to be explicitly specified and quoted.

$ dvc stage add stage_name -o bar -w data -c "echo bar > bar"

@dmpetrov later pointed out that it breaks the autocompletion for users. This discussion was without any DVC's autocompletion installed.

With DVC's autocompletion installed, the autocompletion anyway does not work as it is assumed as a plain string.

Anyway, we have to either choose between-c and the remainder arg in run.

cc @dberenbaum @shcheklein @jorgeorpinel

@skshetry skshetry added enhancement Enhances DVC p1-important Important, aka current backlog of things to do discussion requires active participation to reach a conclusion labels Jan 27, 2021
@skshetry skshetry self-assigned this Jan 27, 2021
@dberenbaum
Copy link
Collaborator

I don't understand the autocompletion issue with the -c "echo bar > bar" format. I'm able to autocomplete on Ubuntu in bash and zsh even inside of quotes. Is there an example of how this breaks autocompletion? Is it a specific issue with OSX? Maybe @dmpetrov can elaborate?

@skshetry
Copy link
Member Author

@dberenbaum, that's strange. For me (on Linux), it never autocompletes, with or without DVC's autocompletion.

@dberenbaum
Copy link
Collaborator

@skshetry What are you trying to autocomplete? What shell are you using? Does autocomplete work for you without the -c flag and quotes?

@dmpetrov
Copy link
Member

dmpetrov commented Jan 27, 2021

@dberenbaum Autocompletion issue - the default autocomplete suggests files that I have in a dir when I type dvc run -n s1 python tr<TAB> (suggests train.py). It won't suggest inside a quoted string - dvc run s1 -c " python tr<TAB> (however, it suggests as 1st item in a string dvc run s1 -c "tr<TAB> which is not a usual corner case).

@skshetry The decision about string VS reminder was made in the very first version of DVC. I considered a -c/--command option as the opposite of the remainder. The reminder looks more common and convenient. An analogy with a reminder - xargs.

To my mind, the issue with mixing up params is not a major one. By replacing the behavior we might have new issues (the autocomplete is one of them). I suggest not changing the behavior and keep cmd as a reminder.

@dberenbaum
Copy link
Collaborator

Thanks, @dmpetrov! I was trying the equivalent of dvc run s1 -c "tr<TAB>, but now I understand and see the same on my end.

@shcheklein
Copy link
Member

I agree that But, we have heard complaints from users that it's confusing, as trying to add certain flags to dvc run at the end is now assumed to be part of the user's script. doesn't sound like a big issue, considering that people will have to double quote commands now (and escaping if needed) + they will have to remember/learn about -c + it breaks autocompletion (not matter DVC's one installed or not).

@dberenbaum
Copy link
Collaborator

dberenbaum commented Jan 27, 2021

Yup, let's stick with the existing syntax (positional arguments) unless you have any concerns @skshetry .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion requires active participation to reach a conclusion enhancement Enhances DVC p1-important Important, aka current backlog of things to do
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants