Skip to content
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

feat: Graceful server shutdown and enforce only one running instance of Phoenix at a time #155

Merged
merged 6 commits into from
Jan 6, 2023

Conversation

nate-mar
Copy link
Contributor

@nate-mar nate-mar commented Jan 5, 2023

Resolves #97

  • Gracefully shuts down the current Session's running instance of the Phoenix Server.
  • In the event that there are other instances of Phoenix running on the host machine and a user attempts to launch the app, any currently running instance will be shut down.

@nate-mar nate-mar changed the title Graceful server shutdown and enforce only one running instance of Phoenix at a time feat: Graceful server shutdown and enforce only one running instance of Phoenix at a time Jan 5, 2023
if len(os.listdir(server.get_pids_path())) > 0:
# Currently, only one instance of Phoenix can be running at any given time.
# Support for multiple concurrently running instances may be supported in the future.
logger.warning("Existing running Phoenix instance detected! Shutting it down...")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to warn and exit instead and force the user to explicitly ask to kill the existing instance?

@@ -37,13 +45,24 @@ def start(self) -> None:
)

def stop(self) -> None:
"""Stops the service."""
"""Gracefully stops the service."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is graceful shutdown necessary? If a user kills any existing Server instance while launching the app, any cleanup will not be performed. However, I don't see a lot of actual clean up being done at the moment.

Comment on lines 8 to 28
def _get_temp_path() -> str:
"""Get path to directory in which to store temp phoenix server files."""
return os.path.join(tempfile.gettempdir(), ".arize-phoenix")


def get_pids_path() -> str:
"""Get path to directory in which to store temp phoenix instance pid files.
This directory is used to track any currently running instances of Arize Phoenix
on the host machine. The directory will be created if it does not exist.
"""
path = os.path.join(_get_temp_path(), "pids")
try:
os.makedirs(path)
except OSError as e:
if e.errno == errno.EEXIST:
pass
else:
raise
else:
os.chmod(path, 0o777)
return path
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a config module here: https://github.com/Arize-ai/phoenix/blob/main/src/phoenix/config.py

I think we should centralize the paths here - should we move dataset files into temp too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually originally had it in there, but I moved it out because I thought it wouldn't generally be something configurable, and would only be relevant to the server. Cool with moving it back, if you just want to centralize.

raise


def _get_pid_file() -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe name it as _pid_file_path? file implies content to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes gp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah shoot, I thought I had changed this. will do it in a follow up

Comment on lines 15 to 29
def _write_pid_file():
with open(_get_pid_file(), "w") as outfile:
pass


def _remove_pid_file() -> None:
try:
os.unlink(_get_pid_file())
except OSError as e:
if e.errno == errno.ENOENT:
# If the pid file doesn't exist, ignore and continue on since
# we are already in the desired end state; This should not happen
pass
else:
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: it sorta feels like the processes should be managed / encapsulated in the session and that the session tears down the processes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can revisit this. The process is associated with the server, rather than the session; so not entirely sure what this would entail.

@nate-mar nate-mar marked this pull request as ready for review January 6, 2023 09:42
@nate-mar nate-mar merged commit dbc18de into main Jan 6, 2023
@nate-mar nate-mar deleted the server-shutdown branch January 6, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[server] Setup a service that can gracefully shut down the web server
2 participants