-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ui: use global singleton UI object for printing #5653
Conversation
@@ -152,9 +152,11 @@ def table(self, header, rows, markdown: bool = False): | |||
self.write(ret) | |||
|
|||
|
|||
if __name__ == "__main__": | |||
ui = Console() | |||
ui = Console() |
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 it should be console
now?
file: TextIO = None, | ||
flush: bool = False, | ||
) -> None: | ||
if self.disabled: | ||
if not self._enabled: |
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.
Q: How important is thread-safety for us while printing? logger
is thread safe, but this is not.
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.
So, ultimately, we are intending to use ui.write
everywhere we use logger
now? If so, I would say thread safety is pretty important as we already have some parts of code that utilize ThreadPoolExecutor
and log something down the line (for example, _upload_plans
.
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.
thanks, @pared. Will come to it as we start using it in multithreaded applications.
@skshetry Could you clarify how we would change verbosity or in general tweak ui in cases like erepo? |
Not implemented, but verbosity could be changed in a similar way. Probably, changing the API of ui.configure(level="success", enable=True) But, I think we should not do that, as I am thinking
I had implemented this before, but removed it to keep API simple. I am thinking of using with ui.enabled():
ui.write("visible")
with ui.disabled():
ui.write("hidden")
with ui.enabled():
ui.write(" visible again")
ui.write(" hidden again") This should be thread and async safe. |
@skshetry Sorry for the delay. Looks pretty good. 👍 |
Okay, merging it for now. |
I understand this is a bit controversial, but it seems that using this
ui
object globally is the best thing to do.DVC tries to provide a sane
API
as well asCLI
. Extending api to accomodateui
object (which is forcli
) does not seem like a good thing, as it extends our API surface and makes it ugly. We'd also need to pass them to the depth of our APIs.Similarly, we cannot just use
ui
object in CLIs, as our cli commands are usually just wrapper to theRepo
apis. So, even if we try hard to separate them and isolate it inside theCLI
, it'd be quite difficult.Looking at it, and the purpose that
ui
object serves, it seems to me that it is a global singleton object anyway (an instance in a single session of CLI). And, we useloggers
in a similar way, so I find it fitting as per our needs, without adding any extra efforts.❗ 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. 🙏