command: Should not consume 'print' to output default remote info - #6650
Conversation
1. should not consume 'print' to output default remote info, otherwise the --quiet would not work.
| conf = self.config.read(self.args.level) | ||
| try: | ||
| print(conf["core"]["remote"]) | ||
| ui.write(conf["core"]["remote"]) |
There was a problem hiding this comment.
@eggqq007, could you please describe your use case? Do you want to check if the default is set or not?
There was a problem hiding this comment.
Yes, I want to check the whether the project has set default-remote or not. And I think it should not output any regular string once --quiet was add.
There was a problem hiding this comment.
We don't treat --quiet as a no output mode. We still output the data that the user asked for, but silence all the other outputs that DVC usually throws (so that dvc metrics diff --show-md still works with --quiet, etc).
This is a bit of an edge-case here as you may want to either check what the default is or test if the default remote has been set.
There was a problem hiding this comment.
Thanks for the reply. emm, I think updating the dvc document in https://dvc.org/doc/command-reference/remote/add is a candidate. It says "-q, --quiet - do not write anything to standard output. Exit with 0 if no problems arise, otherwise 1."
There was a problem hiding this comment.
Current behavior seems consistent with the guideline in #5891. I think it even gives the behavior you are looking for @eggqq007, but it's inverted (there is output printed only if the default remote has been set). The command reference definitely needs an update, though, and probably for other commands also.
There was a problem hiding this comment.
Ahh, okay. I was mostly sticking to the first point of the guideline:
If the command's (or, the option) that sole purpose is to display the information, let show it in any case.
I guess we need to clarify if this belongs to the second one or not:
If the information is very important to the user, print warnings and error messages to the stderr even in --quiet mode wherever appropriate.
I have mostly chosen to err on the side of caution, and support --quiet only if the command's/flag's sole purpose is to show that information, if --quiet has other uses too, I have kept that so far (as we have discussed, --quiet on the dvc metrics diff probably is an error on the user's side).
dvc status does the same thing when used with --quiet, in fact it's what we have been recommending for use in the scripts.
To resolve this, I guess we need to either suggest alternatives here, or clarify the guideline then.
There was a problem hiding this comment.
I'm not following. Isn't the sole purpose of dvc remote default (without any args/options) and dvc status to display information? What do you think dvc remote default --quiet should show according to the guideline?
There was a problem hiding this comment.
Isn't the sole purpose of dvc remote default (without any args/options) and dvc status to display information?
Actually no, dvc status -q without output is used to indicate whether there are any changes, same as git status -q. dvc remote default can be used similarly in scripts - to tell user if it has default remote or not. So I think that making -q suppress it is justifiable beacuse dvc remote default has two purposes: with and without printing.
There was a problem hiding this comment.
@efiop Do you mean git status -s (I don't think git status -q is an option)? Both git status -s and dvc status -q show output if there are changes and suppress output if there are no changes. dvc remote default -q shows output if there is a default remote and suppresses output if there is no default remote.
How is this different from what you are suggesting? I'm still not following what behavior changes you or @skshetry are suggesting.
There was a problem hiding this comment.
Discussed this as a group. I was wrong about dvc status -q, which never prints anything. In that case, it seems fine to do the same here and merge this PR. I don't think it can cause much harm, and it aligns with the current documentation. Sorry for the oversight and all the noise here.
|
Will someone be able to contribute treeverse/dvc.org#2840 too? 🙂 |
| conf = self.config.read(self.args.level) | ||
| try: | ||
| print(conf["core"]["remote"]) | ||
| ui.write(conf["core"]["remote"]) |
There was a problem hiding this comment.
Discussed this as a group. I was wrong about dvc status -q, which never prints anything. In that case, it seems fine to do the same here and merge this PR. I don't think it can cause much harm, and it aligns with the current documentation. Sorry for the oversight and all the noise here.
|
Thanks @eggqq007 🙂 |
* master: (47 commits) make dependabot updates work (treeverse#6760) move defaults to the class level (treeverse#6762) build(deps): Bump dvclive from 0.3.0 to 0.4.0 (treeverse#6753) move requirements to setup.cfg (treeverse#6758) gha: cancel previous workflows (treeverse#6756) pyinstaller: hooks: remove webhdfs webhdfs: migrate to fsspec (treeverse#6662) gha: benchmarks: fix pytest flags (treeverse#6749) hdfs: migrate to fsspec (treeverse#6604) Pin dvclive to 0.3.0 command: Should not consume 'print' to output default remote info (treeverse#6650) command: output prettify json in tty (treeverse#6743) gha: run benchmarks on PRs (treeverse#6733) Run on Python3.10 (treeverse#6745) Fix dvclive running subdir (treeverse#6740) exp init: only ask for that are not provided in an interactive mode (treeverse#6739) move exp init logic to dvc.repo.experiments.init (treeverse#6738) build(deps): Bump jaraco-windows from 5.6.0 to 5.7.0 (treeverse#6735) build(deps): Bump pytest-cov from 2.12.1 to 3.0.0 (treeverse#6736) Bump typing_extensions to >=3.7.4 (treeverse#6731) (treeverse#6732) ...
5 participants
otherwise the --quiet would not work.
❗ 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. ref: update
--quietbehavior dvc.org#2840Thank you for the contribution - we'll try to review it as soon as possible. 🙏