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: quotes are missing in dvc.yaml #6072

Closed
aguschin opened this issue May 27, 2021 · 5 comments · Fixed by #6074
Closed

stage add: quotes are missing in dvc.yaml #6072

aguschin opened this issue May 27, 2021 · 5 comments · Fixed by #6074
Labels
A: cli Related to the CLI bug Did we break something? p1-important Important, aka current backlog of things to do

Comments

@aguschin
Copy link
Contributor

Bug Report

Description

Quotes in dvc stage add aren't added to dvc.yaml as expected. The following dvc repro fails as python script expects to have an empty string provided.

Reproduce

  1. mkdir dvc-bug-empty-string-arg
  2. cd dvc-bug-empty-string-arg
  3. dvc init --no-scm
  4. dvc stage add -n train python train.py --weights ""
  5. dvc stage add -n train-second python train.py --weights '""'
  6. cat dvc.yaml

output:

stages:
  train:
    cmd: 'python train.py --weights '
  train-second:
    cmd: python train.py --weights ""

Expected

Expected to have quotes in the first stage, have two quotes in the second one.

Environment information

Output of dvc doctor:

$ dvc doctor
DVC version: 2.1.0 (brew)
---------------------------------
Platform: Python 3.9.5 on macOS-10.16-x86_64-i386-64bit
Supports: azure, gdrive, gs, http, https, s3, ssh, oss, webdav, webdavs
Cache types: <https://error.dvc.org/no-dvc-cache>
Caches: local
Remotes: None
Workspace directory: apfs on /dev/disk1s5s1
Repo: dvc (no_scm)

Additional Information (if any):

Rechecked this in bash and zsh.

@aguschin aguschin added the bug Did we break something? label May 27, 2021
@skshetry
Copy link
Member

@aguschin, this is expected. You have to escape those quotes explicitly.

@skshetry skshetry added awaiting response we are waiting for your reply, please respond! :) and removed bug Did we break something? labels May 27, 2021
@shcheklein
Copy link
Member

@skshetry could you please provide a bit more context on why is it expected? thanks!

@skshetry
Copy link
Member

We use a argparse.REMAINDER, so that means we have to parse things and quote things properly. And to be cross-compatible, we have to quote with double-quotes rather than single. So, the parsing of those commands get ugly here because of all the quoting that we need to do for earlier arguments, and to solve this empty string that we get from "", we might need to replace that to a double quoted string, which might clash with previous ones.

So, I prefer users to escape things when not working to escape from this ugly parsing stuff. But the fix for this would be to just replace the empty string with a quoted empty string.

skshetry added a commit to skshetry/dvc that referenced this issue May 28, 2021
@skshetry skshetry added bug Did we break something? A: cli Related to the CLI c1-quick-fix p1-important Important, aka current backlog of things to do and removed awaiting response we are waiting for your reply, please respond! :) labels May 28, 2021
@aguschin
Copy link
Contributor Author

aguschin commented Jun 3, 2021

@skshetry, is this also indended?

dvc exp run -S param=
ERROR: Must provide a value for parameter 'param'
# or
dvc exp run -S param=""
ERROR: Must provide a value for parameter 'param'

One may want to provide an empty parameter value, to get

# params.yaml
param:

Checked this on dvc installed from the latest commit in master branch 2.3.0+eb46c0

@skshetry
Copy link
Member

skshetry commented Jun 4, 2021

@skshetry, is this also intended?

For the record, replied in #6107 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: cli Related to the CLI bug Did we break something? p1-important Important, aka current backlog of things to do
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants