-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Terminal colors #905
base: main
Are you sure you want to change the base?
Terminal colors #905
Changes from 4 commits
6d3800f
38fc7be
5a08f12
aaffff2
acea90e
cdb8314
43da50b
0b13ca8
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 |
---|---|---|
|
@@ -17,11 +17,19 @@ | |
import numpy as np | ||
|
||
try: | ||
from IPython import get_ipython | ||
from IPython.display import clear_output, display, HTML | ||
|
||
ipython_is_imported = True | ||
except ImportError: | ||
ipython_is_imported = False | ||
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 we still need this, or is it replaced by |
||
notebook_mode = False | ||
else: | ||
ipython_is_imported = True | ||
_ipython = get_ipython() | ||
notebook_mode = ( | ||
_ipython is not None | ||
and "IPKernelApp" in _ipython.config | ||
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. Based on the way that 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. We had to tackle this detection back in InterpretML too, here's how we did it there: https://github.com/interpretml/interpret/blob/develop/python/interpret-core/interpret/provider/_environment.py. We could re-use some of this logic here perhaps cc @nopdive 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. Sorry, hadn't been watching my notifications -- code seems reasonable. Yes, this should be tested (either manually/automatically is fine) on multiple platforms before merge: terminal (Window/Linux/Mac), vscode, jupyter notebook/lab, azure/google/amazon/databricks notebooks. We should be relatively okay if these target environments work. |
||
) | ||
|
||
try: | ||
import torch | ||
|
||
|
@@ -39,7 +47,7 @@ | |
) | ||
from .. import _cpp as cpp | ||
from ._guidance_engine_metrics import GuidanceEngineMetrics | ||
from .._utils import softmax, CaptureEvents | ||
from .._utils import softmax, CaptureEvents, ModelStateHTMLParser | ||
from .._parser import EarleyCommitParser, Parser | ||
from .._grammar import ( | ||
GrammarFunction, | ||
|
@@ -862,6 +870,8 @@ def __init__(self, engine, echo=True, **kwargs): | |
self._event_parent = None | ||
self._last_display = 0 # used to track the last display call to enable throttling | ||
self._last_event_stream = 0 # used to track the last event streaming call to enable throttling | ||
self._state_html_parser = ModelStateHTMLParser() # used to parse the state for cli display | ||
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. Same as before, consider making naming specific to console if it's only used for console mode. |
||
self._last_state_len = 0 # used to track the last state length for appending to cli display | ||
|
||
@property | ||
def active_role_end(self): | ||
|
@@ -975,11 +985,18 @@ def _update_display(self, throttle=True): | |
else: | ||
self._last_display = curr_time | ||
|
||
if ipython_is_imported: | ||
if notebook_mode: | ||
clear_output(wait=True) | ||
display(HTML(self._html())) | ||
else: | ||
pprint(self._state) | ||
print( | ||
self._state_html_parser.feed( | ||
self._state[self._last_state_len:] | ||
), | ||
end='', | ||
flush=True | ||
) | ||
self._last_state_len = len(self._state) | ||
|
||
def reset(self, clear_variables=True): | ||
"""This resets the state of the model object. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
"openai": ["openai>=1.0"], | ||
"schemas": ["jsonschema"], | ||
"server": ["fastapi", "uvicorn"], | ||
"cli": ["colored"] | ||
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. Not a super widely-used package, so I am open to alternative solutions if there are any concerns. 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've used @nopdive might have thoughts here 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.
We can manually put together a lookup table if we only want to support a few shades from red to green or something like that, but I'll also take a look at some of the other libraries that 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. \begin{mutter}If you refactor so that notebook output and console output are separate implementations of the same base class, this problem largely goes away\end{mutter} 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. The problem still stands of choosing a way to map probabilities to colors, but yes at the very least the problem of parsing a particular rgba value goes away :) 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. @Harsha-Nori, I'm not familiar with |
||
} | ||
|
||
# Create the union of all our requirements | ||
|
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.
If it's only console related, have the name/module relate to console.
On a similar note, might be a good time to consider re-architecting visualization to have sibling classes between notebook and terminal.
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.
@nking-1 and I are taking a look at rethinking how we're representing internal model state in order to make this possible (hence why this PR appears a bit stalled out at the moment). Currently, formatting information is very very closely entangled with internal representation, but yes ideally we can abstract that away