initial pass at controlling model instance and threads for fallback triton servers #41228

wpmccormack · 2023-03-29T23:18:36Z

PR description:

For SONIC, the number of instances of a model in a fallback server and the number of threads that fallback server can occupy on local CPU resources can be controlled via the model's triton config file. We set the number of instances to the number of threads specified for the job, so this implementation requires a bit of text editing before the fallback servers can run. The model files live in cvmfs, and the config files cannot be edited there, so they are pulled locally. The precise edits depend on the model's backend; luckily the backend information can be extracted from the config files, so no additional dictionary containing this information is needed.

PR validation:

This has been tested locally to verify that: 1) the config files get edited correctly, 2) that the fallback servers start properly, and 3) that a miniAOD processing job can run completely.

…riton servers

cmsbuild · 2023-03-29T23:19:05Z

A new Pull Request was created by @wpmccormack (Patrick McCormack) for CMSSW_12_6_X.

It involves the following packages:

HeterogeneousCore/SonicTriton (heterogeneous)

@cmsbuild, @makortel, @fwyzard can you please review it and eventually sign? Thanks.
@makortel, @missirol, @riga, @kpedro88, @rovere this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

makortel

Is the "copy model locally" expected to be a temporary solution, or the long-term solution?

makortel · 2023-03-31T00:37:23Z

HeterogeneousCore/SonicTriton/interface/TritonService.h

+#include "FWCore/ServiceRegistry/interface/SystemBounds.h"
+#include "FWCore/ServiceRegistry/interface/ServiceMaker.h"


The ServiceMaker.h include should be unnecessary, and for SystemBounds.h it would be better to forward-declare edm::service::SystemBounds below.

Removed and forward declared

makortel · 2023-03-31T00:37:42Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

@@ -218,6 +218,7 @@ start_singularity(){
 	for REPO in ${REPOS[@]}; do
 		MOUNTARGS="$MOUNTARGS -B $REPO"
 		REPOARGS="$REPOARGS --model-repository=${REPO}"
+		ls ${REPO}


Is this line a leftover?

Yes sorry, it's been removed now

makortel · 2023-03-31T00:38:10Z

HeterogeneousCore/SonicTriton/plugins/TritonService.cc

@@ -1,4 +1,5 @@
 #include "FWCore/ServiceRegistry/interface/ServiceMaker.h"
+#include "FWCore/ServiceRegistry/interface/SystemBounds.h"


This #include should not be needed.

makortel · 2023-03-31T00:38:24Z

HeterogeneousCore/SonicTriton/src/TritonService.cc

@@ -6,6 +6,8 @@
 #include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
 #include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
 #include "FWCore/ServiceRegistry/interface/ActivityRegistry.h"
+#include "FWCore/ServiceRegistry/interface/SystemBounds.h"
+#include "FWCore/ServiceRegistry/interface/ServiceMaker.h"


The ServiceMaker.h include should not be needed here.

makortel · 2023-03-31T00:38:56Z

HeterogeneousCore/SonicTriton/interface/TritonService.h

@@ -128,12 +131,14 @@ class TritonService {
  unsigned currentModuleId_;
  bool allowAddModel_;
  bool startedFallback_;
+  unsigned int numInstancesForFallbacks_;


This member appears to be unused.

Sorry it's been removed

makortel · 2023-03-31T01:01:29Z

HeterogeneousCore/SonicTriton/src/TritonService.cc

+      out << "parameters { key: \"TF_NUM_INTRA_THREADS\" value: { string_value: \"1\" } }" << std::endl;
+      out << "parameters { key: \"TF_NUM_INTER_THREADS\" value: { string_value: \"1\" } }" << std::endl;
+      out << "parameters { key: \"TF_USE_PER_SESSION_THREADS\" value: { string_value: \"1\" } }" << std::endl;
+    }


What if neither isONNX or isTF is true?

So this is another subtlety with triton server configs: they are model-type dependent. Thread controls are only implemented right now for ONNX and TF models, so if neither of those options are true, then nothing else should be included in the config

We should at least emit a warning in this case. something like: "Warning: thread (instance) control not implemented for $PLATFORM" (following one of my suggestions below)

I added this, thanks!

makortel · 2023-03-31T01:02:49Z

HeterogeneousCore/SonicTriton/src/TritonService.cc

+    bool isONNX = false;
+    bool isTF = false;
+
+    size_t offset;


Looks like the offset is not really needed.

This has all been deleted

makortel · 2023-03-31T01:04:51Z

HeterogeneousCore/SonicTriton/src/TritonService.cc

+    bool isTF = false;
+
+    size_t offset;
+    std::string line;


Please move this declaration inside the if block as it is needed only there.

Yes, all deleted and moved to cmsTriton script

makortel · 2023-03-31T01:05:09Z

HeterogeneousCore/SonicTriton/src/TritonService.cc

+
+    size_t offset;
+    std::string line;
+    std::ifstream Myfile;


Variable names should start with lower case character.

makortel · 2023-03-31T01:08:57Z

HeterogeneousCore/SonicTriton/src/TritonService.cc

+    }
+
+    //append instance and thread information to config files
+    std::ofstream out;


I'd suggest to explicitly close the out file. My personal preference would be to place all the code accessing out inside a block along

{ std::ofstream out{"./local_model_repo/" + token + "/config.pbtxt", std::ios::app}; // all code accessing out }

this is no longer done here. Now I just echo in the cmsTriton bash script

kpedro88 · 2023-03-31T14:42:32Z

HeterogeneousCore/SonicTriton/src/TritonService.cc

@@ -238,7 +247,67 @@ void TritonService::preBeginJob(edm::PathsAndConsumesOfModulesBase const&, edm::
  if (fallbackOpts_.wait >= 0)
    fallbackOpts_.command += " -w " + std::to_string(fallbackOpts_.wait);
  for (const auto& [modelName, model] : unservedModels_) {
-    fallbackOpts_.command += " -m " + model.path;
+    //We need to edit the triton config files, need to copy from cvmfs


I would actually suggest to handle the copying and editing of model config files in the cmsTriton script (by passing in the numberOfThreads value when the script is called). Two reasons:

most operations here are shell commands, therefore more naturally suited to be implemented in bash

allows this functionality (controlling number of instances per model) to be used outside of CMSSW/TritonService

Ah yes good suggestion, thank you! Let me switch that over

kpedro88 · 2023-03-31T14:49:08Z

Is the "copy model locally" expected to be a temporary solution, or the long-term solution?

We do not expect a different way to do this in the foreseeable future. It's possible at some point that Triton's model configuration interface changes so these settings can be specified "on the fly", but that's not on the current roadmap.

…ce.cc

cmsbuild · 2023-03-31T22:04:36Z

Pull request #41228 was updated. @cmsbuild, @makortel, @fwyzard can you please check and sign again.

kpedro88 · 2023-03-31T22:10:35Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+    echo "]" >> "$CONFIG"
+    echo "" >> "$CONFIG"
+
+    if grep -Fq "onnx" "$CONFIG"


to try to maintain a consistent style in this script, a few things to fix:

indent with tabs (currently a mix of tabs and spaces in newly added lines, which is the worst of all possibilities)

format if statements as:
if condition; then action fi

Fixed. Sorry, didn't notice that my emacs was doing space instead of tabs when I was pressing "tab"

kpedro88 · 2023-03-31T22:11:19Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

@@ -315,14 +319,59 @@ wait_server(){
 	echo "server is ready!"
 }

+edit_model(){
+    cp -r $1 $TMPDIR/local_model_repo/


to make the code a little easier to read/follow, I would start out by assigning $1, $2, etc. to local variables and then use the local variables everywhere.

kpedro88 · 2023-03-31T22:17:57Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+    IFS='/' read -ra ADDR <<< "$1"
+    CONFIG=$TMPDIR/local_model_repo/${ADDR[-1]}/config.pbtxt
+
+    echo "" >> "$CONFIG"


this would be a little easier to read (and a little more efficient) if cat (heredoc) were used, e.g.

cmssw/FWCore/Integration/test/test_finalpath.sh

Lines 19 to 27 in 3d13002

cat <<EOF > finalpath_expected_not_found.log

did not find thing '' TEST

did not find thing '' TEST

did not find thing '' TEST

found thing 'beginLumi' TEST

found thing 'endLumi' TEST

found thing 'beginRun' TEST

found thing 'endRun' TEST

EOF

(the downside is that the text body can't be indented to match the rest of the function, but I think it's a worthwhile tradeoff)

I switched to this format

kpedro88 · 2023-03-31T22:25:17Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+	echo "parameters { key: \"inter_op_thread_count\" value: { string_value: \"1\" } }" >> "$CONFIG"
+    fi
+
+    if grep -Fq "tensorflow" "$CONFIG"


this should be an elif (there's no case where we would apply both TF and ONNX settings to the same file)

also, I would check for the backend type more specifically, something like this:

PLATFORM=$(grep -m 1 "^platform:" "$CONFIG") if [[ $PLATFORM == *"tensorflow"* ]]; then ... fi

Done! Thanks for the code snippet

kpedro88 · 2023-03-31T22:25:49Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+
+    if grep -Fq "onnx" "$CONFIG"
+    then
+	echo "parameters { key: \"intra_op_thread_count\" value: { string_value: \"1\" } }" >> "$CONFIG"


as above, use cat/heredoc (also avoids need to escape quotes)

kpedro88 · 2023-03-31T22:26:18Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

 list_models(){
 	# make list of model repositories
+        if [ "$INSTANCES" -gt 0 ]
+	then
+	    if [ -d "$TMPDIR/local_model_repo" ]


put the name "local_model_repo" in a local variable somewhere to reduce duplication/hardcoding

I put this in a global variable so that it can be used by both list_models and edit_model without being forwarded from one to the other. Is that ok? I can switch to a local variable if you'd prefer

That's fine.

kpedro88 · 2023-03-31T22:26:53Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+	    then
+		#Want to start with a fresh copy of model files in case this directory already exists with local edits
+		rm -rf $TMPDIR/local_model_repo
+		mkdir $TMPDIR/local_model_repo


this line is also in the "else" block, so just remove the else block and put this line outside the if statement

kpedro88 · 2023-03-31T22:29:32Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+		if [ "$INSTANCES" -gt 0 ]
+		then
+		    edit_model $MODEL "$INSTANCES"
+		    REPOS=$TMPDIR/local_model_repo


just assign to REPOS once outside of (after) the loop over models

kpedro88 · 2023-03-31T22:30:17Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

 	for ((r=0; r < ${#REPOS[@]}; r++)); do
 		# avoid issues w/ multiple levels of symlinks


This part (extending till the end of the function) may not be necessary for the local_model_repo case

I moved this into an if statement that is also related to the above comment

kpedro88 · 2023-03-31T22:30:26Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

 	list_models

-	make_tmp
+	#make_tmp


remove commented-out code

cmsbuild · 2023-04-03T19:20:35Z

Pull request #41228 was updated. @cmsbuild, @makortel, @fwyzard can you please check and sign again.

kpedro88 · 2023-04-03T20:19:25Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+EOF
+
+
+	#if grep -Fq "onnx" "$CONFIG"; then


delete commented-out code

kpedro88 · 2023-04-03T20:19:28Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

+parameters { key: "inter_op_thread_count" value: { string_value: "1" } }
+EOF
+
+	#elif grep -Fq "tensorflow" "$CONFIG"; then


delete commented-out code

cmsbuild · 2023-04-03T21:51:15Z

Pull request #41228 was updated. @cmsbuild, @makortel, @fwyzard can you please check and sign again.

kpedro88 · 2023-05-23T18:20:50Z

HeterogeneousCore/SonicTriton/scripts/cmsTriton

@@ -50,6 +52,7 @@ usage() {
 	$ECHO "-r [num]    \t number of retries when starting container (default: ${RETRIES})"
 	$ECHO "-s [dir]    \t Singularity sandbox directory (default: $(get_sandbox))"
 	$ECHO "-t [dir]    \t non-default hidden temporary dir"
+	$ECHO "-I [num]    \t number of model instances (default: ${INSTANCES} -> means no local editing of config files)"


to keep alphabetical order, move this up to L47

Moved it; not sure why I didn't have it alphabetical to begin with

cmsbuild · 2023-06-01T17:07:54Z

Pull request #41228 was updated. @cmsbuild, @makortel, @fwyzard can you please check and sign again.

makortel · 2023-06-01T18:31:40Z

@kpedro88 Do you have any further comments?

wpmccormack · 2023-06-01T18:33:13Z

(Oh, I forgot to mention that we validated this PR by reproducing some previous results where thread controls were performed manually)

kpedro88 · 2023-06-01T18:58:04Z

@makortel this is good to go from my side.

makortel · 2023-06-05T06:57:17Z

@cmsbuild, please test

cmsbuild · 2023-06-05T16:10:52Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_6_X IBs (but tests are reportedly failing) and once validation in the development release cycle CMSSW_13_2_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

wpmccormack · 2023-06-05T16:12:31Z

ah... looks like this PR is targeting the wrong branch. @wpmccormack , you should change the target branch to master (and then we should retest it).

I tried switching the target branch to master, and it said that ~200 files had been changed. I started this work from 12_6_4, not master I think. Unless there's an easier way to do things, I'll just start a new PR. Is that ok?

cmsbuild · 2023-06-05T16:20:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41228/35789

makortel · 2023-06-05T16:22:32Z

Thanks for spotting @kpedro88.

@wpmccormack While technically changing the target branch and updating your triton_model_instance_control branch with a rebase on top of any 13_[012]_0_preX prerelease (e.g. 13_2_0_pre1 for the most recent one) should work (one also needs to be faster than @cmsbuild between the two operations to avoid tagging unrelated reviewers), a new PR is fine as well.

perrotta · 2023-06-05T21:04:59Z

Replaced by #41876, which is correctly based on master.
Let close this PR, then

wpmccormack added 2 commits March 29, 2023 18:06

initial pass at controlling model instance and threads for fallback t…

7c61b00

…riton servers

fix formatting issue

a1e98c8

cmsbuild added this to the CMSSW_12_6_X milestone Mar 29, 2023

cmsbuild added pending-signatures tests-pending orp-pending heterogeneous-pending labels Mar 29, 2023

makortel reviewed Mar 31, 2023

View reviewed changes

kpedro88 reviewed Mar 31, 2023

View reviewed changes

wpmccormack added 2 commits March 31, 2023 17:02

moving triton config editing to cmsTriton script from src/TritonServi…

9e4f739

…ce.cc

formatting fix from code checks

a45df41

kpedro88 reviewed Mar 31, 2023

View reviewed changes

addressing Kevin's comments - mostly stylistic updates for cmsTriton

45e12dc

kpedro88 reviewed Apr 3, 2023

View reviewed changes

removing commented out code

518ade3

kpedro88 reviewed May 23, 2023

View reviewed changes

making cmsTriton options alphabetical

38b55b4

wpmccormack marked this pull request as ready for review June 1, 2023 17:08

cmsbuild added tests-started and removed tests-pending labels Jun 5, 2023

cmsbuild added this to the CMSSW_12_6_X milestone Jun 5, 2023

cmsbuild added pending-signatures heterogeneous-pending and removed fully-signed heterogeneous-approved labels Jun 5, 2023

wpmccormack mentioned this pull request Jun 5, 2023

Adding automated thread controls for fallback triton servers #41876

Merged

perrotta closed this Jun 5, 2023

		#include "FWCore/ServiceRegistry/interface/SystemBounds.h"
		#include "FWCore/ServiceRegistry/interface/ServiceMaker.h"

		@@ -1,4 +1,5 @@
		#include "FWCore/ServiceRegistry/interface/ServiceMaker.h"
		#include "FWCore/ServiceRegistry/interface/SystemBounds.h"

	cat <<EOF > finalpath_expected_not_found.log
	did not find thing '' TEST
	did not find thing '' TEST
	did not find thing '' TEST
	found thing 'beginLumi' TEST
	found thing 'endLumi' TEST
	found thing 'beginRun' TEST
	found thing 'endRun' TEST
	EOF

		for ((r=0; r < ${#REPOS[@]}; r++)); do
		# avoid issues w/ multiple levels of symlinks

initial pass at controlling model instance and threads for fallback triton servers #41228

initial pass at controlling model instance and threads for fallback triton servers #41228

Conversation

wpmccormack commented Mar 29, 2023

PR description:

PR validation:

cmsbuild commented Mar 29, 2023

makortel 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

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

kpedro88 commented Mar 31, 2023

cmsbuild commented Mar 31, 2023

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

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

cmsbuild commented Apr 3, 2023

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

cmsbuild commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jun 1, 2023

makortel commented Jun 1, 2023

wpmccormack commented Jun 1, 2023

kpedro88 commented Jun 1, 2023

makortel commented Jun 5, 2023

cmsbuild commented Jun 5, 2023

wpmccormack commented Jun 5, 2023

cmsbuild commented Jun 5, 2023

makortel commented Jun 5, 2023 • edited Loading

perrotta commented Jun 5, 2023

makortel commented Jun 5, 2023 •

edited

Loading