-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[WIP] log: use bright/dim colors for ui #3835
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
Changes from all commits
b590dbf
e893366
26572e4
dbfe38a
d5424ce
e5828b5
7c15229
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |||||
| import dvc.dependency as dependency | ||||||
| import dvc.prompt as prompt | ||||||
| from dvc.exceptions import CheckoutError, DvcException | ||||||
| from dvc.utils import relpath | ||||||
| from dvc.utils import relpath, styled | ||||||
|
|
||||||
| from . import params | ||||||
| from .decorators import rwlocked | ||||||
|
|
@@ -77,7 +77,7 @@ def create_stage(cls, repo, path, **kwargs): | |||||
| logger.warning("Build cache is ignored when persisting outputs.") | ||||||
|
|
||||||
| if not ignore_run_cache and stage.can_be_skipped: | ||||||
| logger.info("Stage is cached, skipping.") | ||||||
| logger.info("Stage is cached, skippingβ¦") | ||||||
| return None | ||||||
|
|
||||||
| return stage | ||||||
|
|
@@ -209,10 +209,11 @@ def changed_deps(self): | |||||
| return False | ||||||
|
|
||||||
| if self.is_callback: | ||||||
| logger.warning( | ||||||
| '{stage} is a "callback" stage ' | ||||||
| logger.debug( | ||||||
| '%s is a "callback" stage ' | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| "(has a command and no dependencies) and thus always " | ||||||
| "considered as changed.".format(stage=self) | ||||||
| "considered as changed.", | ||||||
| self, | ||||||
| ) | ||||||
| return True | ||||||
|
|
||||||
|
|
@@ -248,9 +249,9 @@ def changed_outs(self): | |||||
|
|
||||||
| return False | ||||||
|
|
||||||
| def changed_stage(self, warn=False): | ||||||
| def changed_stage(self): | ||||||
| changed = self.md5 != self.compute_md5() | ||||||
| if changed and warn: | ||||||
| if changed: | ||||||
| logger.debug(self._changed_stage_entry()) | ||||||
| return changed | ||||||
|
|
||||||
|
|
@@ -259,14 +260,12 @@ def changed(self): | |||||
| is_changed = ( | ||||||
| # Short-circuit order: stage md5 is fast, | ||||||
| # deps are expected to change | ||||||
| self.changed_stage(warn=True) | ||||||
| self.changed_stage() | ||||||
| or self.changed_deps() | ||||||
| or self.changed_outs() | ||||||
| ) | ||||||
| if is_changed: | ||||||
| logger.info("%s changed.", self) | ||||||
| else: | ||||||
| logger.info("%s didn't change.", self) | ||||||
| logger.debug("%s changed.", self) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return is_changed | ||||||
|
|
||||||
| @rwlocked(write=["outs"]) | ||||||
|
|
@@ -294,8 +293,11 @@ def remove(self, force=False, remove_outs=True): | |||||
|
|
||||||
| @rwlocked(read=["deps"], write=["outs"]) | ||||||
| def reproduce(self, interactive=False, **kwargs): | ||||||
|
|
||||||
| if not (kwargs.get("force", False) or self.changed()): | ||||||
| logger.info( | ||||||
| "Stage '%s' didn't change, skippingβ¦", | ||||||
| styled(self.addressing, "bold"), | ||||||
| ) | ||||||
| return None | ||||||
|
|
||||||
| msg = ( | ||||||
|
|
@@ -375,6 +377,10 @@ def save_outs(self): | |||||
| for out in self.outs: | ||||||
| out.save() | ||||||
|
|
||||||
| def ignore_outs(self): | ||||||
| for out in self.outs: | ||||||
| out.ignore() | ||||||
|
|
||||||
| @staticmethod | ||||||
| def _changed_entries(entries): | ||||||
| return [str(entry) for entry in entries if entry.changed_checksum()] | ||||||
|
|
@@ -548,8 +554,8 @@ def _status_stage(self, ret): | |||||
| if self.cmd_changed: | ||||||
| ret.append("changed command") | ||||||
|
|
||||||
| def changed_stage(self, warn=False): | ||||||
| if self.cmd_changed and warn: | ||||||
| def changed_stage(self): | ||||||
| if self.cmd_changed: | ||||||
| logger.debug(self._changed_stage_entry()) | ||||||
| return self.cmd_changed | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |||||
| import subprocess | ||||||
| import threading | ||||||
|
|
||||||
| from dvc.utils import fix_env | ||||||
| from dvc.utils import fix_env, styled | ||||||
|
|
||||||
| from .decorators import unlocked_repo | ||||||
| from .exceptions import StageCmdFailedError | ||||||
|
|
@@ -79,11 +79,16 @@ def cmd_run(stage, *args, **kwargs): | |||||
|
|
||||||
|
|
||||||
| def _is_cached(stage): | ||||||
| return ( | ||||||
| cached = ( | ||||||
| not stage.is_callback | ||||||
| and not stage.always_changed | ||||||
| and stage.already_cached() | ||||||
| ) | ||||||
| if cached: | ||||||
| logger.info( | ||||||
| "Stage %s is cached, skippingβ¦", styled(stage.addressing, "bold"), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ) | ||||||
| return cached | ||||||
|
|
||||||
|
|
||||||
| def restored_from_cache(stage): | ||||||
|
|
@@ -93,7 +98,13 @@ def restored_from_cache(stage): | |||||
| return False | ||||||
| # restore stage from build cache | ||||||
| stage_cache.restore(stage) | ||||||
| return stage.outs_cached() | ||||||
| restored = stage.outs_cached() | ||||||
| if restored: | ||||||
| logger.info( | ||||||
| "Restored stage %s from run-cache, skippingβ¦", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Is run-cache (hyphenated) the official term we will be using consistently? |
||||||
| styled(stage.addressing, "bold"), | ||||||
| ) | ||||||
| return restored | ||||||
|
|
||||||
|
|
||||||
| def run_stage(stage, dry=False, force=False, run_cache=False): | ||||||
|
|
@@ -102,10 +113,17 @@ def run_stage(stage, dry=False, force=False, run_cache=False): | |||||
| run_cache and restored_from_cache(stage) | ||||||
| ) | ||||||
| if stage_cached: | ||||||
| logger.info("Stage is cached, skipping.") | ||||||
| stage.checkout() | ||||||
| return | ||||||
|
|
||||||
| logger.info("Running command:\n\t%s", stage.cmd) | ||||||
| callback_str = ( | ||||||
| "{} ".format(styled("callback", "bold")) if stage.is_callback else "" | ||||||
| ) | ||||||
| logger.info( | ||||||
| "Running %s" "stage %s with command:", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| callback_str, | ||||||
| styled(stage.addressing, "bold"), | ||||||
| ) | ||||||
| logger.info("\t%s", stage.cmd) | ||||||
|
skshetry marked this conversation as resolved.
|
||||||
| if not dry: | ||||||
| cmd_run(stage) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,6 +270,17 @@ def colorize(message, color=None): | |
| ) | ||
|
|
||
|
|
||
| def styled(message, style=None): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @skshetry this awesome and long waited from my end ... the only question - how well it supported by older/SSH terminals, Windows, etc - we should do some fallback to regular colors? or will this logic handle this automatically? (also, really wanted to improve the way we print the box around telemetry, updater messages - there was some concern with older terminals as well that prevented us from using nice symbols - may be we can detect it also)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I see that we only change brightness here, also as a good next step we can start using normal modern colors instead of RED, BLUE - etc that are even hard to read sometimes - again the biggest question is compatibility and fallbacks)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should work on Windows as it uses colorama, but not sure of others.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was using colors before, like I showed yesterday. But, I moved away from it, because if other log appears in between, mix of colored and non-colored output looks weird. Plus, the colors that work on both dark/light terminals need to be choosen (most probably, red/green/yellow).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
do you mean when we use
Sorry, I was confusing. Your are right about using brightness for these logs. Everything else I mentioned is not directly related to this PR. So, please feel free to disregard :) I was saying about using "pastel" terminal colors instead of regular RED, BLUE - e.g. https://github.com/tartley/colorama/blob/master/colorama/ansi.py#L60 - for all other logs. Same as Also using some nice looking symbols for the box instead of a regular dash.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No. I mean, as we use INFO for ui, we can either support only one color (colored or not) or manually change all to whatever color we like. But, any
I did gave a quick look to https://github.com/chalk/supports-color, looks like it's possible. But, do we really need True colors (i.e. 256 colors are enough for us? At least for a start).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 256 is totally enough. (again, not really related to this PR, I would suggest a ticket for this)
not sure I completely follow ... but looks like another reason to not use logger for UI. |
||
| if not style: | ||
| return message | ||
| styles = { | ||
| "bold": colorama.Style.BRIGHT, | ||
| "normal": colorama.Style.NORMAL, | ||
| "dim": colorama.Style.DIM, | ||
| } | ||
| return f"{styles[style]}{message}{styles['normal']}" | ||
|
|
||
|
|
||
| def boxify(message, border_color=None): | ||
| """Put a message inside a box. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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: Do we use
β¦(ellipsis character) a lot? I'm guessing it's mostly...(3 period chars) in other files, if any.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.
Also