-
Notifications
You must be signed in to change notification settings - Fork 73
Queue up log items when Client is not ready #36
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
|
Cool @znation! This is definitely an improvement over the current behavior, but it's still somewhat blocking as it relies on the timeout from Screen.Recording.2025-06-10.at.6.04.06.PM.movInstead, I wonder if we can offload the Client instantiation to a background thread, and have self._client_ready = threading.Event()
self._client_lock = threading.Lock()
self._client = client # Might be None
self.queued_logs = deque()
if self._client is None:
threading.Thread(target=self._init_client_background, daemon=True).start()
else:
self._client_ready.set()
def _init_client_background(self):
try:
client = Client(self.url, verbose=False)
with self._client_lock:
self._client = client
self._client_ready.set()
except Exception as e:
print(f"[trackio] Failed to initialize client in background: {e}")
# Do not set _client_ready, keep it false so log() knows to queueOne other thing to keep in mind is that the experiment might finish before the Client has instantiated so perhaps we should also, in the background thread, log everything that is currently in the queue, after the Client has instantiated. In this case, we might be accessing the queue from multiple threads, so we should wrap it with a |
|
@abidlabs I debated doing that (see comment thread) but I guess I should. I'll update this PR to use a background thread and drain the queue when the client is instantiated. |
|
I don't think we need a |
|
Thanks @znation sorry I missed the thread earlier, but this approach you suggested sounds good to me |
fd6380c to
a5598fb
Compare
|
@abidlabs I implemented the |
|
Hmm I'm seeing a error when running e.g. |
|
Thanks for finding that @abidlabs, apparently I should test scripts in addition to in a Jupyter notebook. I'll fix that. |
|
Thanks @abidlabs, I think with this commit it should work from both a script and Jupyter notebook. In order to avoid errors and data loss we have to block at some point and make sure the client is instantiated, so I have done this on the |
|
Nice @znation! Code looks good I tested with The only thing I noticed was that it kept printing In the end, my console looked like this: imo it'd be better to suppress these. otherwise, lgtm |
|
Just a heads up that which we should queue |
Instead of blocking while waiting for a Space to initialize, keep the client as None and attempt to initialize it lazily on each log call. When a client is successfully initialized, the log call will flush the queue. Fixes gradio-app#28 ruff format ruff check --fix ruff check --fix Init client on background thread ruff check --fix ruff format Update trackio/run.py Co-authored-by: Abubakar Abid <abubakar@huggingface.co> Update trackio/run.py Co-authored-by: Abubakar Abid <abubakar@huggingface.co> Wait for background thread on `finish` In case we still haven't created the client by the time `finish` is called, block and wait for the thread to finish which will flush the queue. Otherwise it's likely a script will exit without having flushed the queue. ruff format
d0713e2 to
dd1415f
Compare
Instead of blocking while waiting for a Space to initialize, keep the client as None and attempt to initialize it lazily on each log call. When a client is successfully initialized, the log call will flush the queue.
Fixes #28