Improved handling of SIGTERM #660

ugermann · 2020-06-17T15:06:59Z

Description

moved signal handling out of scheduler code (was already global there, but still in the files)
pay attention to the signal in the threads that prepare batches in the background, which can take quite a while and prevented Marian from shutting down quickly after receiving SIGTERM in some circumstances

How to test

start training, then send SIGTERM to process, it should save the current state of affairs and exit

Checklist

I have tested the code manually
I have run regression tests
I have read and followed CONTRIBUTING.md
I have updated CHANGELOG.md

When marian receives signal SIGTERM and exits gracefully (save model & exit), it should then exit with a non-zero exit code, to signal to any parent process that it did not exit "naturally".

…atus after SIGTERM.

…marian-dev into ug-graceful-shutdown

…ning. Prior to this bug fix, BatchGenerator::fetchBatches, which runs in a separate thread, would ignore SIGTERM during training (training uses a custom signal handler for SIGTERM, which simply sets a global flag, to enable graceful shutdown (i.e., save models and current state of training before shutting down). The changes in this commit also facilitate custom handling of other signals in the future by providing a general singal handler for all signals with a signal number below 32 (setSignalFlag) and a generic flag checking function (getSignalFlag(sig)) for checking such flags.

…code easier This does not introduce any new functionality, just moves code around, so that future PRs are easier to compare. Moving old GraphGroup code to training/deprecated. Once it is clear there is nothing in there that's worth saving, this will be deleted. Replace -Ofast with -O3 and make sure ffinite-math is turned off.

…in/max quantization to avoid overflow 1. Change the weight matrix quantization to use 7-bit min/max quantization -> This resolves all the overflow issue, because weight and activations are quantized by min/max range. 2. Clip fp16 quantization to avoid overflow 3. Fix windows build errors (cmake options, vcproj file) 4. int8 pack model (encoder -> fp16)

…encoders as well For int8 quantized model, use int8 quantization for encoders as well. The quality difference between fp16 encoder and int8 encoder is small, but they have quite amount of speed difference.

* Add basic support for TSV inputs * Fix mini-batch-fit for TSV inputs * Abort if shuffling data from stdin * Fix terminating training with data from STDIN * Allow creating vocabs from TSV files * Add comments; clean creation of vocabs from TSV files * Guess --tsv-size based on the model type * Add shortcut for STDIN inputs * Rename --tsv-size to --tsv-fields * Allow only one 'stdin' in --train-sets * Properly create separate vocabularies from a TSV file * Clearer logging message * Add error message for wrong number of valid sets if --tsv is used * Use --no-shuffle instead of --shuffle in the error message * Fix continuing training from STDIN * Update CHANGELOG * Support both 'stdin' and '-' * Guess --tsv-fields from dim-vocabs if special:model.yml available * Update error messages * Move variable outside the loop * Refactorize utils::splitTsv; add unit tests * Support '-' as stdin; refactorize; add comments * Abort if excessive field(s) in the TSV input * Add a TODO on passing one vocab with fully-tied embeddings * Remove the unit test with excessive tab-separated fields

* fix 0 * nan behavior in concatention * bump patch * change epsilon to margin

* Refactorize processPaths * Fix relative paths for shortlist and sqlite options * Rename InterpolateEnvVars to interpolateEnvVars * Update CHANGELOG

…anch Cherry pick a few improvements/fixes from Frank's branch * Adds Frank's fix for label-based mini-batch sizing from Frank's current experimental branch. * Also copies minor improvements and a few comments.

* python3 shebang from #620 * Add changelog entry for python3 change

* Fix server build with current boost, move simple-websocket-server to submodule * Change submodule to marian-nmt/Simple-WebSocket-Server * Update submodule simple-websocket-server Co-authored-by: Gleb Tv <[email protected]>

* Use cblas_sgemm_batch when available * Merge with master, add comments and describe contribution

…ning. Prior to this bug fix, BatchGenerator::fetchBatches, which runs in a separate thread, would ignore SIGTERM during training (training uses a custom signal handler for SIGTERM, which simply sets a global flag, to enable graceful shutdown (i.e., save models and current state of training before shutting down). The changes in this commit also facilitate custom handling of other signals in the future by providing a general singal handler for all signals with a signal number below 32 (setSignalFlag) and a generic flag checking function (getSignalFlag(sig)) for checking such flags.

…in/max quantization to avoid overflow 1. Change the weight matrix quantization to use 7-bit min/max quantization -> This resolves all the overflow issue, because weight and activations are quantized by min/max range. 2. Clip fp16 quantization to avoid overflow 3. Fix windows build errors (cmake options, vcproj file) 4. int8 pack model (encoder -> fp16)

emjotde

Good work. Thanks.

src/common/config_parser.cpp

src/common/signal_handling.cpp

emjotde · 2020-08-26T19:35:13Z

src/data/batch_generator.h

-      if (bufferedBatches_.empty() // i.e., end of Epoch
-          || getSignalFlag(SIGTERM)) { // process received SIGTERM, abandon ship ...
-        return nullptr;
+      if(runAsync_) { // by default we will run in asynchronous mode


Comment to self and @frankseide: this is git being bad at doing a three-way diff. The runAsync_ part is from the master branch.

@ugermann I see what you meant with the 3-way diff.

src/data/batch_generator.h

emjotde · 2020-08-26T19:39:52Z

@frankseide this looks good to me now and ready for another look from you.

Cosmetic fixes as per code review. - removed superfluous empty line - but default value for --sigterm on a new line

src/common/config_parser.cpp

@snukky

Move option --segterm to General Options for training, as suggested by @snukky.

Remove log message from signal handler. Note that the log still records that training was interrupted by a signal both in the log message in Scheduler::finished() and in the exit code at the end of marian_train.cpp.

Fix formatting of --sigterm option specification.

frankseide

Minor feedback.

One is that the term "graceful" is not concise. E.g. users who send SIGTERM do not request a graceful exit. Marian gracefully exits (i.e. cleans up GPU resources etc.) when training is done. What the user requests is an early exit. Let us rethink the naming.

frankseide · 2020-08-27T19:25:32Z

src/common/config_parser.cpp

+    // (e.g., dump model but continue training upon SIGUSR1, or report current
+    // training status upon SIGINFO.)
+    cli.add<std::string>("--sigterm",
+      "What to do with SIGTERM: 'graceful' => save and exit (default); 'immediate' => exit immediately.", 


"graceful" is not descriptive. Please say in the option string what it does, e.g. "save-and-exit"

"immediate" could be "terminate"

Strings changed, but SIGTERM is widely considered a request for a graceful shutdown.
See here: https://qph.fs.quoracdn.net/main-qimg-1180ef2465c309928b02481f02580c6a
and here: https://www.booleanworld.com/kill-process-linux/#:~:text=By%20default%2C%20all%20the%20process,gracefully%22.
and here: python-trio/snekomatic#6

In all of these examples, graceful shutdown means to leave the world in a well-defined state (e.g. finish in-flight requests to a service). The examples do not suggest saving state, and note that the PR saves state in a way that is inconsistent with configured behavior, because the configuration says to save every save-freq updates, and not at a time of receiving SIGTERM, which is a random update.

In my mind, you don't want a "graceful shutdown" as given in these examples, but more like an "early exit" from the training process.

Alternatively, you may be wanting "preemption," i.e. save state that allows to restart the training. Your PR accomplishes that as well (right?), and such feature would be useful in our internal server farm as well when running jobs in low-priority preemptable mode.

Saving state to resume later is what Microsoft incidentally calls 'graceful' preemption, as opposed to 'immediate' preemption: https://docs.microsoft.com/en-us/powershell/high-performance-computing/understanding-policy-configuration?view=hpc19-ps#:~:text=Immediate%20preemption%20(Default)%3A%20Take,allocated%20to%20another%20job%20immediately.&text=Graceful%20preemption%3A%20Take%20resources%20from,that%20work%20is%20not%20lost.

frankseide · 2020-08-27T19:29:44Z

src/common/signal_handling.cpp

+bool getSignalFlag(const int sig) {
+  // sig_atomic_t has 32 bits. We don't accommodate signals beyond that.
+  ABORT_IF(sig >= 32, "Signal out of range (must be < 32, is {}).", sig);
+  return sigflags_ & (1<<sig);


& is an integer operation, but you return bool.

Can you please also explain what this function does? It seems it does two things: It converts the signal, but then also masks it with a class member. I cannot understand the meaning of the function without a comment.

Explanation added in code.

frankseide · 2020-08-27T19:31:32Z

src/common/signal_handling.cpp

+
+bool getSignalFlag(const int sig) {
+  // sig_atomic_t has 32 bits. We don't accommodate signals beyond that.
+  ABORT_IF(sig >= 32, "Signal out of range (must be < 32, is {}).", sig);


Do we really need this check? I suggest to remove both the comment and the ABORT, since sig_atomic_t is, by its name, clearly meant for signals, and therefore defines the valid signal ranges. Marian is not an operating system, we don't need to hedge against every possible invalid argument. You can also remove the check and comment in setSignalFlag

sig_atomic_t is meant for signals in the sense that certain operations on it are guaranteed to be atomic even during signal handling. Essentially it is an int type that I'm using here as a bit field, so that we have a generic signal handler that we can install for arbitrary signals (lower than 30). That check is a check against programming errors. Signals can be higher than 30, but this code does not accommodate installing setSignalFlag for such signals.

frankseide · 2020-08-27T19:32:12Z

src/common/signal_handling.cpp

+
+void requestGracefulExit(int sig) {
+  setSignalFlag(sig);         // keep track of triggering signal
+  gracefulExitRequested_ = 1; // set flag to exit gracefully


gracefulExitRequested() returns a bool, but here you set it to an integer. Please choose one (bool if possible)

This relies on implicit type conversion from int types to bool. sig_atomic_t is specifically meant to be used in signal handlers. This is run-of-the-mill signal handling, see "Compliant Solution (Writing volatile sig_atomic_t)" here: https://wiki.sei.cmu.edu/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers.

src/common/signal_handling.h

frankseide · 2020-08-27T19:34:32Z

src/common/signal_handling.h

+
+/// General purpose signal handler that simply sets a flag when a signal is received.
+//  (only for SIGNAL No. < 32).
+void setSignalFlag(const int sig);  // custom handler (set flag) for sig


I suggest to not use const in these function signatures. It exposes an internal behavior of the function to the caller. And the functions are so simple that adding const does not make them easier to udnerstand or verify.

Const removed.

frankseide · 2020-08-27T19:38:38Z

src/training/training.h

@@ -16,6 +16,7 @@ template <class ModelWrapper>
 class Train : public ModelTask {
 private:
  Ptr<Options> options_;
+  void installCustomSignalHandlers();


Is it installing multiple signal handlers?

Potentially in the future. I prefer to keep API changes rare.

Please rename to installEarlyExitSignalHandler()

I strongly disagree on this one. installCustomSignalHandlers() is like parseOptions() a generic concept: install whatever custom signal handlers may be specified in the options. Either my pull request goes in as-is now, or I'll withdraw it. @emjotde

- Update comments - Explicit conversion from int to bool in functions that return bool.

ugermann · 2020-08-28T14:13:55Z

@frankseide I think I have now addressed all your concerns.

frankseide

Please rename "graceful exit" to "early exit," as "graceful exit" means something else. After that change, I agree to merge.

CHANGELOG.md

src/common/signal_handling.cpp

src/training/scheduler.h

frankseide · 2020-08-28T18:02:44Z

src/training/training.h

@@ -16,6 +16,7 @@ template <class ModelWrapper>
 class Train : public ModelTask {
 private:
  Ptr<Options> options_;
+  void installCustomSignalHandlers();


Please rename to installEarlyExitSignalHandler()

ugermann · 2020-08-28T18:43:54Z

@frankseide renamed 'graceful exit' to 'save and exit'.

emjotde · 2020-08-31T23:26:55Z

@frankseide can you take another look if you concerns are satisfied? If yes, please approve, I will merge then.

frankseide

One last change, then this can go in.

src/training/scheduler.h

emjotde · 2020-09-01T01:56:15Z

I will rename this quickly and pull it in.

emjotde · 2020-09-01T02:14:48Z

@snukky I love the new continuous integration :)

emjotde · 2020-09-01T02:43:47Z

Aren't the checks a necessary condition before merging? Now it allows me to merge despite the checks not having finished yet after my update? Should that not be a blocker?

emjotde · 2020-09-01T04:14:21Z

@ugermann merged, thanks.

ugermann and others added 30 commits November 19, 2019 22:49

Return exit code 15 (SIGTERM) after SIGTERM.

73bdb1f

When marian receives signal SIGTERM and exits gracefully (save model & exit), it should then exit with a non-zero exit code, to signal to any parent process that it did not exit "naturally".

Added explanatory comment about exiting marian_train with non-zero st…

653b13d

…atus after SIGTERM.

Merge branch 'ug-graceful-shutdown' of https://github.com/marian-nmt/…

8a44759

…marian-dev into ug-graceful-shutdown

bump version

dd06542

Merged PR 12243: For int8 quantized model, use int8 quantization for …

d0fa14e

…encoders as well For int8 quantized model, use int8 quantization for encoders as well. The quality difference between fp16 encoder and int8 encoder is small, but they have quite amount of speed difference.

bump version

c95676e

actually save the merge file

71cc43a

use float values for catch::Approx

09904e0

Fix TSV training with mini-batch-fit after the last merge

4d12ffa

Update submodule regression-tests

855c94a

fix 0 * nan behavior in concatention

c18fc71

Fix 0 * nan behavior due to using -O3 instead of -OFast (#630)

0ba438c

* fix 0 * nan behavior in concatention * bump patch * change epsilon to margin

Update submodule regression-tests

93a27dc

Support relative paths in shortlist and sqlite options (#612)

733cb50

* Refactorize processPaths * Fix relative paths for shortlist and sqlite options * Rename InterpolateEnvVars to interpolateEnvVars * Update CHANGELOG

Fix Iris example on CPU (#623)

7bf486a

Dump version

34bc47c

Merged PR 12442: cherry pick a few improvements/fixes from Frank's br…

bc8b6fa

…anch Cherry pick a few improvements/fixes from Frank's branch * Adds Frank's fix for label-based mini-batch sizing from Frank's current experimental branch. * Also copies minor improvements and a few comments.

update changelog and version

ce94fe9

python3 shebang from #620 (#621)

59dad14

* python3 shebang from #620 * Add changelog entry for python3 change

Update submodule regression-tests

342db58

Batched gemm (#633)

9ae1951

* Use cblas_sgemm_batch when available * Merge with master, add comments and describe contribution

update version

1603d2f

Update submodule regression-tests

ae1dd47

emjotde approved these changes Aug 26, 2020

View reviewed changes

Update config_parser.cpp

6a753d6

Cosmetic fixes as per code review. - removed superfluous empty line - but default value for --sigterm on a new line

snukky reviewed Aug 26, 2020

View reviewed changes

src/common/config_parser.cpp Outdated Show resolved Hide resolved

ugermann added 3 commits August 26, 2020 21:32

Update config_parser.cpp

deafe6a

Move option --segterm to General Options for training, as suggested by @snukky.

Update signal_handling.cpp

8e41f22

Remove log message from signal handler. Note that the log still records that training was interrupted by a signal both in the log message in Scheduler::finished() and in the exit code at the end of marian_train.cpp.

Update config_parser.cpp

69b10cf

Fix formatting of --sigterm option specification.

frankseide requested changes Aug 27, 2020

View reviewed changes

ugermann added 6 commits August 27, 2020 22:32

Fix trailing whitespace.

1a5fbbc

Cosmetic cleanup.

506eb71

Comment code.

139052a

Change values for --sigterm in response to code review.

56e38b3

Bug fix in variable name.

c7ad4f1

Update signal_handling.cpp.

8d50a85

- Update comments - Explicit conversion from int to bool in functions that return bool.

frankseide requested changes Aug 28, 2020

View reviewed changes

ugermann added 2 commits August 28, 2020 19:27

Rename 'graceful exit' to 'save and exit'.

0406025

Update CHANGELOG.md.

d7044f7

ugermann changed the title ~~Improved signal handling for graceful shutdown~~ Improved handling of SIGTERM Aug 28, 2020

frankseide approved these changes Sep 1, 2020

View reviewed changes

src/training/scheduler.h Outdated Show resolved Hide resolved

rename function name to saveAndExitRequested()

6a4d887

emjotde merged commit 044b416 into master Sep 1, 2020

snukky mentioned this pull request Sep 1, 2020

Add GitHub workflow with CUDA 11 on Ubuntu 20.04 #704

Merged

4 tasks

snukky deleted the ug-graceful-shutdown branch January 24, 2022 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved handling of SIGTERM #660

Improved handling of SIGTERM #660

ugermann commented Jun 17, 2020 •

edited

Loading

emjotde left a comment

emjotde Aug 26, 2020

emjotde Aug 26, 2020

emjotde commented Aug 26, 2020

frankseide left a comment

frankseide Aug 27, 2020

ugermann Aug 27, 2020

frankseide Aug 27, 2020

ugermann Aug 28, 2020

frankseide Aug 27, 2020

ugermann Aug 28, 2020

frankseide Aug 27, 2020

ugermann Aug 28, 2020

frankseide Aug 27, 2020

ugermann Aug 28, 2020

frankseide Aug 27, 2020

ugermann Aug 28, 2020

frankseide Aug 27, 2020

ugermann Aug 27, 2020

frankseide Aug 28, 2020

ugermann Aug 28, 2020

ugermann commented Aug 28, 2020

frankseide left a comment

frankseide Aug 28, 2020

ugermann commented Aug 28, 2020

emjotde commented Aug 31, 2020 •

edited

Loading

frankseide left a comment

emjotde commented Sep 1, 2020

emjotde commented Sep 1, 2020

emjotde commented Sep 1, 2020

emjotde commented Sep 1, 2020

Improved handling of SIGTERM #660

Improved handling of SIGTERM #660

Conversation

ugermann commented Jun 17, 2020 • edited Loading

Description

How to test

Checklist

emjotde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emjotde commented Aug 26, 2020

frankseide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ugermann commented Aug 28, 2020

frankseide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ugermann commented Aug 28, 2020

emjotde commented Aug 31, 2020 • edited Loading

frankseide left a comment

Choose a reason for hiding this comment

emjotde commented Sep 1, 2020

emjotde commented Sep 1, 2020

emjotde commented Sep 1, 2020

emjotde commented Sep 1, 2020

ugermann commented Jun 17, 2020 •

edited

Loading

emjotde commented Aug 31, 2020 •

edited

Loading