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

Run Orion in the background #7057

Closed
wants to merge 26 commits into from
Closed

Run Orion in the background #7057

wants to merge 26 commits into from

Conversation

hallenmaia
Copy link
Contributor

@hallenmaia hallenmaia commented Oct 3, 2022

Summary

Adds prefect orion start --detach and prefect orion stop commands.

Closes #6989 and #6821

prefect orion start --detach:

Run Orion in the background. It can also be called using the shortcut -d

prefect orion start -d
Orion running in background.
Check out the dashboard at http://127.0.0.1:4200

A PID file is written to the path ~/.prefect/orion.pid (depending on how PREFECT_HOME is set) so that it can be stopped.

An error is displayed when this file already exists and the start command is used:

prefect orion start
There is already an Orion process running in background.
Stop it with command `prefect orion stop`

prefect orion stop:

Checks if the file ~/.prefect/orion.pid (depending on how PREFECT_HOME is configured) exists and terminates the background Orion process.

prefect orion stop    
Stopping Orion...
Orion stopped!

Displays an error if this file does not exist or is invalid. Example:

prefect orion stop
Orion is not running in the background.

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a label categorizing the change e.g. fix, feature, enhancement

@netlify
Copy link

netlify bot commented Oct 3, 2022

Deploy Preview for prefect-orion ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8765d5b
🔍 Latest deploy log https://app.netlify.com/sites/prefect-orion/deploys/63440828b18cf2000986f74c
😎 Deploy Preview https://deploy-preview-7057--prefect-orion.netlify.app/api-ref/prefect/utilities/processutils
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

src/prefect/cli/orion.py Outdated Show resolved Hide resolved
src/prefect/cli/orion.py Outdated Show resolved Hide resolved
src/prefect/cli/orion.py Outdated Show resolved Hide resolved
src/prefect/cli/orion.py Outdated Show resolved Hide resolved
hallenmaia and others added 3 commits October 4, 2022 09:00
Co-authored-by: Michael Adkins <contact@zanie.dev>
@hallenmaia
Copy link
Contributor Author

hallenmaia commented Oct 4, 2022

@madkinsz I improved the error messages. The following tasks are pending:

  • Create test_orion.py to improve coverage.
  • Make the start command synchronous.
  • Real Test on Windows. (There is no CI for Windows)

If there's anything else let me know.

@hallenmaia
Copy link
Contributor Author

hallenmaia commented Oct 4, 2022

encode/uvicorn#1579

CTRL+C doesn't work on Uvicorn for Windows :(
Perhaps the start command has to remain asynchronous.

@hallenmaia
Copy link
Contributor Author

Tests run on Windows with version:

Version:             0+untagged.12757.ga5c67cb
API version:         0.8.1
Python version:      3.10.7
Git commit:          a5c67cb0
Built:               Wed, Oct 5, 2022 10:36 AM
OS/Arch:             win32/AMD64
Profile:             default
Server type:         ephemeral
Server:
  Database:          sqlite
  SQLite version:    3.37.2

The prefect orion start -d and prefect orion stop commands worked perfectly. The error mentioned in #6821 does not occur with --detach.

@hallenmaia
Copy link
Contributor Author

For now it will not be possible to change the start command to asynchronous because there is a problem with propagating exceptions with uvicorn.

encode/uvicorn#1579

On POSIX systems it can be synchronous but on Windows systems CTRL+C is not sent to the uvicorn process. In the comment encode/uvicorn#1579 (comment) it is recommended to leave the process asynchronous to work around this issue.

@hallenmaia hallenmaia marked this pull request as ready for review October 5, 2022 14:07
@zanieb
Copy link
Contributor

zanieb commented Oct 6, 2022

Thanks so much for digging into this!

To clarify, the broken case here is prefect orion start without --detach cannot be stopped by Ctrl-C on Windows? All other cases are working?

I thought that the signal handling was fixed by @bunchesofdonald in #6672 but perhaps that doesn't work with the synchronous process implementation?

Do you see a clear path to retaining the async process implementation while returning before the process exists?

As an alternative, we could retain the async implementation for the blocking case and only use the sync implementation for --detach?

@hallenmaia
Copy link
Contributor Author

To clarify, the broken case here is prefect orion start without --detach cannot be stopped by Ctrl-C on Windows? All other cases are working?

Yup. CTRL+C works in all cases, but on Windows only if the function is asynchronous.

I thought that the signal handling was fixed by @bunchesofdonald in #6672 but perhaps that doesn't work with the synchronous process implementation?

That's right. It was fixed in asynchronous mode. In synchronous mode subprocess sends all signals from the parent process to the children by default. It is not necessary to use signals for this.

But there is a problem in uvicorn's implementation of asyncio that is affecting this behavior on Windows.

I did the following test: I ran the uvicorn command in the Windows terminal inside the Prefect virtual environment and tried to close it using CTRL+C several times. As it didn't close I had to use the task manager for that.

Do you see a clear path to retaining the async process implementation while returning before the process exists?

I did not understand that question.

As an alternative, we could retain the async implementation for the blocking case and only use the sync implementation for --detach?

Yup. I believe this is the best solution until the uvicorn fix comes out.

@zanieb
Copy link
Contributor

zanieb commented Oct 6, 2022

:D by

Do you see a clear path to retaining the async process implementation while returning before the process exists?

I mean, can we use the previous async open_process implementation for both --detach and the blocking start? I tried briefly but it would not return until the subprocess exited. If it's possible, it'd be nice not to split it into two implementations. Otherwise, I guess we'll split it into a detach implementation (sync) and a blocking implementation (async).

@hallenmaia
Copy link
Contributor Author

Now I get it! :)

It is possible by removing the context manager from open_process. When you use the anyio.open_process function with context managers via the with statement it waits for the sub-process to finish before continuing.

I agree not to split it into two implementations and follow the AnyIO documentation:

To run an external command with one call, use run_process().

When you have more complex requirements for your interaction with subprocesses, you can launch one with open_process().

I'll go back to PR for draft and test it that way.

@hallenmaia hallenmaia marked this pull request as draft October 6, 2022 18:29
@hallenmaia hallenmaia marked this pull request as ready for review October 10, 2022 13:43
@hallenmaia
Copy link
Contributor Author

Now closes #6821

@hallenmaia
Copy link
Contributor Author

I merged the functions and now they have the following purposes:

run_process: To run an external command with a call.

open_process: When you have more complex requirements for your interaction with subprocesses.

Windows

The start command works in both attached and detached mode. Issue #6821 been fixed.

For detached mode, there is an issue with the ProactorEventLoop implementation reported here which causes an exception to be thrown after the command goes into the background but this is not affecting the functioning of uvicorn (and other commands).

We have a few ways to go:

  • Do not allow Orion to go in the background on Windows;
  • Create two functions: async_open_process and open_process (synchronous) since in synchronous mode this exception does not occur.

@zanieb
Copy link
Contributor

zanieb commented Oct 20, 2022

Sweet! This is coming along nicely.

Regarding Windows, I'm fine with users needing Python 3.10+ to not see the error. Especially since it does not actually impact the server.

I'm a little worried about leaving the async resources unclosed on cancellation. This was a big pain for me to fix previously and I do not want regressions. I think we also need async support for the process killing functionality (that is using psutil). Otherwise, we'll block the event loop / need to run in a worker thread. I'm going to do some investigation into these.

@hallenmaia
Copy link
Contributor Author

hallenmaia commented Oct 20, 2022

Thank you very much. 😊

Sometimes I think it makes no sense to have a detached process asynchronously 🧐. That would avoid some problems.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 60 days with no activity. To keep this pull request open remove stale label or comment.

@github-actions github-actions bot added the status:stale This may not be relevant anymore label Mar 30, 2023
@hallenmaia hallenmaia closed this by deleting the head repository Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:stale This may not be relevant anymore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add prefect orion start --detach and prefect orion stop
2 participants