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

changing default WorkerContext #327

Closed
belm0 opened this issue Sep 28, 2022 · 7 comments · Fixed by #328
Closed

changing default WorkerContext #327

belm0 opened this issue Sep 28, 2022 · 7 comments · Fixed by #328

Comments

@belm0
Copy link

belm0 commented Sep 28, 2022

It would be nice to have a way to change the default WorkerContext, for example:

trio_parallel.set_default_context(WorkerContext(idle_timeout=math.inf))

I'd like to change the idle timeout globally. Currently, I'd have to open a worker context at the root nursery, and store that into a contextvar or something, and hope that everyone remembers to use that context.

The default parameters of WorkerContext are quite arbitrary, so I think it would make sense to have a way to change the default context.

@richardsheridan
Copy link
Owner

Certainly the defaults are arbitrary, and in my main use-case I do exactly as you suggest, throwing the context into the module globals. It works, but it's definitely awkward.

The trouble with modifying the default context is the cross-run shared semantics that I imposed on it. A subsequent (or concurrent!) trio run will inherit whatever default context has been assigned, and the code running at that time may be expecting the default behaviors. Some kind of guard rail for this case would be nice.

During the construction of the WorkerContext feature (#127) I played around with making the whole shebang public and mutable. But, documenting and defining the entire API surface kind of got out of hand, so I hid most everything again. (I would have liked to hide more: #89).

Nevertheless, maybe we can find some minimal way to change the parameters that isn't too confusing or destructive. Can you try in your own application simply assigning to the private global variable before startup:

import trio_parallel._impl
trio_parallel._impl.DEFAULT_CONTEXT = WorkerContext._create(**options)

and let me know if it has any weird knock-on effects for you? If all's well then I can cook up an API to do this in a way that preserves platform-parity (that code won't work on windows) and also ensures every process spawned by the previous default context gets reaped.

@belm0
Copy link
Author

belm0 commented Sep 28, 2022

A subsequent (or concurrent!) trio run will inherit whatever default context has been assigned, and the code running at that time may be expecting the default behaviors. Some kind of guard rail for this case would be nice.

I think it would be OK for set_default_context() to just assert that there is no open trio.run() or active workers. That should cover the common case, where you just want to configure things before using trio/trio_parallel, and hopefully not that hard to implement.

Until someone says "I need to change the default config on the fly", and is going to fund it, there is no need to take it on...

@belm0
Copy link
Author

belm0 commented Sep 28, 2022

trio_parallel._impl.DEFAULT_CONTEXT = WorkerContext._create(**options)

seems to work OK for now, thank you

@richardsheridan
Copy link
Owner

Could you check #328 as well?

@belm0
Copy link
Author

belm0 commented Sep 30, 2022

#328 seems to work OK, thank you

It was a little frustrating that configure_default_context() must be outside the run() context, while mutating current_default_worker_limiter() must be inside a run() context-- so init of global things must be divided into a few places.

@richardsheridan
Copy link
Owner

After a bit of refactoring, it now works inside a trio run. (Actually only inside a trio run on Windows!) The main thread restriction is also a bit artificial, it's just so I don't have to worry about thread races. Any thoughts about that?

I can cut a beta release to pypi if you are interested in moving quickly on this.

@belm0
Copy link
Author

belm0 commented Oct 1, 2022

thank you-- I tried the update, setting from trio.run(), and it works OK

I'll probably keep the old workaround until there is a regular release, no need for a beta in pypi on this end

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 a pull request may close this issue.

2 participants