Skip to content

Andrew/issue 383 fix hang#384

Merged
ayjayt merged 11 commits into
masterfrom
andrew/issue-383-fix-hang
Aug 19, 2025
Merged

Andrew/issue 383 fix hang#384
ayjayt merged 11 commits into
masterfrom
andrew/issue-383-fix-hang

Conversation

@ayjayt
Copy link
Copy Markdown
Collaborator

@ayjayt ayjayt commented Aug 13, 2025

Fixes #383

Adds missing argument that allows python to close without awaiting thread.

Also clarifies that the object is a singleton and it can only be started once. However, adds more functions to silence related warnings.

Also adds the atexit concept from mytien.

@ayjayt ayjayt marked this pull request as draft August 13, 2025 20:22
@ayjayt
Copy link
Copy Markdown
Collaborator Author

ayjayt commented Aug 13, 2025

  • Needs test
  • Is this API really ok?

@emilykl
Copy link
Copy Markdown
Collaborator

emilykl commented Aug 14, 2025

@ayjayt The pattern of always emitting a warning but silencing it under some circumstances feels backwards to me.

Instead, could the open() and close() functions be modified to accept an argument which controls whether or not to emit a warning?

@ayjayt
Copy link
Copy Markdown
Collaborator Author

ayjayt commented Aug 14, 2025

Yeah in general i'm not 100% sure this is the Best API but the warning is only emited either way when the user is calling Open on an already opened instance or close on an already closed instance

As for arguments, yes

@emilykl
Copy link
Copy Markdown
Collaborator

emilykl commented Aug 14, 2025

Right yes by "always" emit a warning I mean always emit when the server is already running.

@ayjayt ayjayt added the needs discussion needs decision on how to fix label Aug 18, 2025
@ayjayt ayjayt marked this pull request as ready for review August 19, 2025 18:33
Comment thread src/py/kaleido/__init__.py Outdated
Comment on lines +18 to +35
def safe_start_sync_server(*args, **kwargs):
"""
Start a kaleido server which will process all sync generation requests.

Only one server can be started at a time.
The kaleido server is a singleton, so it can't be opened twice. This
function will simply return if the server is already running.

This wrapper function takes the exact same arguments as kaleido.Kaleido(),
except one extra, `silence_warnings`.

Args:
*args: all arguments `Kaleido()` would take.
silence_warnings: (bool, default False): If True, emit warning if
starting an already started server.
**kwargs: all keyword arguments `Kaleido()` would take.

This wrapper function takes the exact same arguments as kaleido.Kaleido().
"""
_global_server.open(*args, **kwargs)
_global_server.ensure_opened(*args, **kwargs)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ayjayt This function can be completely removed now, right? And where is stop_sync_server()?

Comment thread src/py/kaleido/__init__.py Outdated
_global_server.open(*args, **kwargs)


__all__ = [
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: this should be at the top

@ayjayt ayjayt added active and removed needs discussion needs decision on how to fix labels Aug 19, 2025
@ayjayt ayjayt merged commit 582127f into master Aug 19, 2025
4 checks passed
@ayjayt ayjayt deleted the andrew/issue-383-fix-hang branch August 19, 2025 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test new sync server API hanging:

2 participants