Skip to content

Add require_main_process to run #4613

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

davetapley
Copy link

@davetapley davetapley commented Apr 12, 2025

This allowed NiceGUI to be started programmatically in a separate SpawnContext.Process, as part of a larger application.

See also:

@evnchn
Copy link
Collaborator

evnchn commented Apr 12, 2025

Hello @davetapley. Thanks for the PR.

Although you most definitely are submitting it because it solved a problem of yours, I'm quite a bit hesitant towards this.

Seems like we are making a check optional, and I am not sure of the side effects of what will happen from that. So, I'd like to talk with you for a bit first.

  1. Importing nicegui affects mp.set_start_method, throws context has already been set #3644 (comment)

You have mentioned that it is no longer reproducible upstream using uvicorn 0.34.0, and we have seen above it work with 0.30.6.

However, #3644 (comment) @rodja commented that he can reproduce using a probably-older but unknown version of uvicorn.

Would you mind going through older uvicorn releases to find which release (and better, which commit) that lead to the fix, so that we can better assess what was changed and whether we should follow suit? Hint: think about what's the latest uvicorn available at the time.

  1. Importing nicegui affects mp.set_start_method, throws context has already been set #3644 (comment)

@falkoschindler reply implies that it would have worked with force=True anyways. It's concerning here that you also used force=True to illustrate the effectiveness of your PR.

Would you be so kind as to test, for completeness sake, with / without the patch, force=True present / absent (4 combinations), and put the results in a table?

@davetapley
Copy link
Author

davetapley commented Apr 13, 2025

@evnchn thanks for the feedback.

  1. I can try and identify the uvicorn version, but I'm not not sure it matters with respect to this PR because: So long as someone is starting nicegui in a child thread then multiprocessing.current_process().name is not going to be 'MainProcess', and so run will exit prematurely. I.e. the problem this PR solves would have existed even if the upstream uvicorn issue had never existed.

  2. Yes force=True is required regardless. I'm not sure of the semantics, but I assume the RuntimeError: context has already been set (which necessitates force=True) is thrown if set_start_method is ever called a second time. For what its worth in my application I've had force=True for several years of production use and it has been fine.


I'll let you know if I encounter any weirdness, but so far it seems to be behaving predictably including building the whole app with PyInstaller (for which I owe hooks-contrib my hook file).

@evnchn
Copy link
Collaborator

evnchn commented Apr 14, 2025

Interesting. Let me conclude for a bit.

We have 2 separate issues, here:

  1. So long as someone is starting nicegui in a child thread, ui.run will exit prematurely. (addressed in this PR)
  2. Require force=True for mp.set_start_method, else throws error. (addressed in Importing nicegui affects mp.set_start_method, throws context has already been set #3644)

Your PR seem to address issue 1 and issue 1 only. It does not do anything with regards to issue 2.

If that's the case, then finding which release that uvicorn throws error on mp.set_start_method without force=True is indeed not related, as we are using force=True anyways.

@davetapley see if I got you right this time. If not, @ me right away with a reply.


Then, some questions just to make sure:

  1. Can you test the following: Does reload=True/False still work? Does native=True/False still work? (4 combinations). These are in-my-opinion the most "impactful" parameters of ui.run, which shakes up dramatically how the NiceGUI app is launched.
  2. Can you, somehow, run the test cases in pytest, so as to see if anything breaks when we make this check optional?
  3. Does it now mean that, if require_main_process=False, and ui.run is not put inside a main guard, the code would blow up? Previously, ui.run can be inside a main guard or not (iirc) and it'd still be fine.

Other than that, LGTM.

@rodja
Copy link
Member

rodja commented Apr 14, 2025

Sorry, @davetapley but can you please sumarize your problem for me? Do you have example code for when you need to require_main_process=False? And how is that related to spawn vs fork?

@falkoschindler falkoschindler added the question Status: Needs clarification from the author label Apr 14, 2025
@davetapley
Copy link
Author

davetapley commented Apr 14, 2025

@rodja sure! I'm programmatically starting nicegui on a new process from inside my existing app (I'm using it to build an admin interface, the app is currently controlled just with config files and a CLI).

Basically like this:

from multiprocessing.context import SpawnContext
from nicegui import ui

class GuiRunnerProcess(SpawnContext.Process):
    def run(self):
        ui.run(reload=False, require_main_process=False, show=False)


if __name__ == "__main__":
    set_start_method("spawn", force=True) # see below for why
    gui_process =  GuiRunnerProcess()
    gui_process.start()

    # rest of processes I start up in my application

I regret mentioning force=true / #3644 in the PR 😁

It's only related insomuch as if you are doing what I'm doing you will additionally need force=true if you are using set_start_method (which I am because I want consistency across platforms, rather than defaulting to fork on Linux).

@evnchn
Copy link
Collaborator

evnchn commented Apr 14, 2025

Read #189 in more details.

It seems like it is also a mix of 2 points:

  1. Whether a main process check was needed (conclusion: yes, for empowering reload=True)
  2. How should such a main process check be done (conclusion: settle on if multiprocessing.current_process().name != 'MainProcess':)

Sorry but I focused on issue 2 when I first read #189.

With point 1, I'd like to ask, would it make more sense and less hassle, if we bypass the main process check if and only if reload=False? *Edit: it should be no reload = single instance = bypass the main process check

From #189 (comment), it appears that "the if-statement in ui.run prevents the spawned process from starting another server".

There won't be another process which is spawned in the first place, if reload=False.

Then, essentially, we are baking in the "optimal defaults" for the require_main_process of ui.run, which is True when reload=True, False when reload=False

@davetapley
Copy link
Author

@evnchn yep, that makes total sense. Thanks for clarifying.

If good with @rodja I will update my PR to remove require_main_process and update the early return check to:

if reload and multiprocessing.current_process().name != 'MainProcess':

@falkoschindler
Copy link
Contributor

@davetapley Unfortunately this brakes run.cpu_bound in combination with reload=False:

from nicegui import ui, run
import time

def compute_sum(a: float, b: float) -> float:
    time.sleep(1)  # simulate a long-running computation
    return a + b

async def handle_click():
    result = await run.cpu_bound(compute_sum, 1, 2)
    ui.notify(f'Sum is {result}')

ui.button('Compute', on_click=handle_click)

ui.run(reload=False)

The child process is not "MainProcess", so we want to exit early from ui.run.

@falkoschindler falkoschindler added analysis Status: Requires team/community input and removed question Status: Needs clarification from the author labels Apr 16, 2025
@evnchn
Copy link
Collaborator

evnchn commented Apr 16, 2025

Interesting observation.

It would work, though, if ui.run was inside a main guard.

from nicegui import ui, run
import time

def compute_sum(a: float, b: float) -> float:
    time.sleep(1)  # simulate a long-running computation
    return a + b

async def handle_click():
    result = await run.cpu_bound(compute_sum, 1, 2)
    ui.notify(f'Sum is {result}')

ui.button('Compute', on_click=handle_click)

if __name__ == "__main__": # put it inside a main guard
    ui.run(reload=False, port=9999)

So, perhaps the solution would be:

  • Normally, we have the main process check, to remain compatible for everyone else.
  • Whoever sets require_main_process = False must bear the responsibility that they put ui.run inside a main guard.

See if that makes sense, and enables @davetapley to do what he wants to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Status: Requires team/community input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants