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

RFC : Added interrupt_workers #3819

Merged
merged 1 commit into from
Aug 2, 2013
Merged

RFC : Added interrupt_workers #3819

merged 1 commit into from
Aug 2, 2013

Conversation

amitmurthy
Copy link
Contributor

Patch provides a means to support Ctrl-C for both remote as well as local workers as outlined in #3805

Base.interrupt_workers() must be called from the sigint handler as desired.

Interrupting workers has been implemented via callbacks to the ClusterManager. The system pid of the worker process is sent back as a response to the initial :join_pgrp message sent to the workers

@JeffBezanson
Copy link
Member

Great functionality to have.

I'm a little concerned about splitting up information about a Worker so that some of it is in the Worker object, and some of it is in dictionaries in the ClusterManager. And a function accepting a symbol to tell it what to do is a little strange; seems like it should be 3 methods instead.

I think the API should be interrupt(workers()) and interrupt(n).

@amitmurthy
Copy link
Contributor Author

I put it in the RegularCluster in order to keep it similar to Scyld, SGE and other Cluster Managers. Since the typical use cases would be ssh or local procs, I could move the required information into the Worker object and only call the ClusterManager callback if it is not of type RegularCluster. Will update the patch.

The function accepting the symbol is a callback function provided by the ClusterManager interface. Currently it is called only on 3 events - register, interrupt and deregister. There may be more events in the future - I just wanted to avoid a series of differently named callbacks. For SGE/Scyld the interrupt functionality will have to be provided by the respective ClusterManagers.

You mean interrupt(; pids = workers()) and interrupt(pid::Integer), right? I'll not export these if this functionality will only be exposed via Ctrl-C

@JeffBezanson
Copy link
Member

Putting in a special case for RegularCluster instead of using the normal callback sounds worse to me.

@amitmurthy
Copy link
Contributor Author

It does keep the regular code cleaner and simpler. Let me quickly try it out and we can then take a call.

@amitmurthy
Copy link
Contributor Author

Removed dictionaries in the cluster manager to keep context. Instead, at worker creation time, the cluster managers can provide an opaque "context" object linked with each worker which is passed back in the callbacks.

The above simplifies the code a bit and also keeps the same model for RegularCluster as well as other ClusterManagers.

@amitmurthy
Copy link
Contributor Author

Have exported interrupt.

interrupt(workers()) uses @sync and @async. I don't know if this may cause problems when called from within a signal handler as may be required for closing #3805

Do let me know if I should change it.

@amitmurthy
Copy link
Contributor Author

@JeffBezanson - can I merge this?

@JeffBezanson
Copy link
Member

OK, let's rebase and merge this.
I'm not sure pids needs to be a keyword argument, but that's a small detail.

amitmurthy added a commit that referenced this pull request Aug 2, 2013
@amitmurthy amitmurthy merged commit 5d05a48 into JuliaLang:master Aug 2, 2013
@ViralBShah
Copy link
Member

Now that this is in, it would be nice to have docs too.

KristofferC pushed a commit that referenced this pull request Feb 29, 2024
Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: master
Julia branch: master
Old commit: 1f16df404
New commit: 48eea8dbd
Julia version: 1.12.0-DEV
Pkg version: 1.11.0(Does not match)
Bump invoked by: @KristofferC
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@1f16df4...48eea8d

```
$ git log --oneline 1f16df404..48eea8dbd
48eea8dbd setenv -> addenv (#3819)
```

Co-authored-by: Dilum Aluthge <[email protected]>
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
…#53517)

Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: master
Julia branch: master
Old commit: 1f16df404
New commit: 48eea8dbd
Julia version: 1.12.0-DEV
Pkg version: 1.11.0(Does not match)
Bump invoked by: @KristofferC
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@1f16df4...48eea8d

```
$ git log --oneline 1f16df404..48eea8dbd
48eea8dbd setenv -> addenv (JuliaLang#3819)
```

Co-authored-by: Dilum Aluthge <[email protected]>
mkitti pushed a commit to mkitti/julia that referenced this pull request Apr 13, 2024
…#53517)

Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: master
Julia branch: master
Old commit: 1f16df404
New commit: 48eea8dbd
Julia version: 1.12.0-DEV
Pkg version: 1.11.0(Does not match)
Bump invoked by: @KristofferC
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@1f16df4...48eea8d

```
$ git log --oneline 1f16df404..48eea8dbd
48eea8dbd setenv -> addenv (JuliaLang#3819)
```

Co-authored-by: Dilum Aluthge <[email protected]>
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.

3 participants