-
Notifications
You must be signed in to change notification settings - Fork 1.3k
command: add dvc push instruction after dvc repro #4737
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
Conversation
Adds a logger.info() statement to remind the user to run dvc push after running dvc repro. Fixes #2359
pared
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvfe I am sorry for such a late response. The change looks good, though I have a suggestion about phrasing. Don't feel obliged to comply, I am not a native speaker.
dvc/command/repro.py
Outdated
| logger.info(_show_metrics(metrics)) | ||
|
|
||
| logger.info( | ||
| "To save your updates to remote storage, run 'dvc push'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like "Use 'dvc push' to send your updates to remote storage."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, done!
dvc/command/repro.py
Outdated
| logger.info( | ||
| "Use 'dvc push' to send your updates to remote storage." | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more things:
- please use ` instead of ' - to keep consistency with other help messages
- I think we should move this message few lines up and log it under
if len(stages)==0inelsecause, so that we will not get this message when there is nothing actually reproduced.
What do you think @jvfe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't really understand what you mean by using "instead of" in this context, could you explain further?
Also, sounds good putting an else statement below if len(stages)==0, like this, right?
if len(stages) == 0:
logger.info(CmdDataStatus.UP_TO_DATE_MSG)
else:
logger.info(
"Use 'dvc push' to send your updates to remote storage."
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvfe I mean backticks ( ` ) instead of ticks ( ' ) - sorry its hard to highlit them, due to markdown formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for explaining! I've done what you suggested but had to split the message in two lines or the pre-commit hook for flake8 would complain.
|
Seems like builds are failing for an unrelated reason. |
| "Use `dvc push` to send your updates to " | ||
| "remote storage." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Should we update some of the console examples in https://dvc.org/doc/command-reference/repro? Several show the command's output.
p.s. looks like that could fit in a single-line-string in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, but there is a problem:
In the example, in repro we modify code only, without influence on the output of the stage...
Seems to me like we should not display this help message there. So I would say we should not update the command reference but fix the underlying issue.
Adds a logger.info() statement to remind the user to run dvc push after
running dvc repro.
Closes #2359
β 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.
Thank you for the contribution - we'll try to review it as soon as possible. π