Predictive Unit/Deployment Environment Variables not set by operator

[axsaucedo]

Currently there are three parameters which should be set by the operator but are not being set, including:

• PREDICTIVE_UNIT_ID
• PREDICTOR_ID
• SELDON_DEPLOYMENT_ID

These three seem are only set if the environment variable for the PREDICTIVE_UNIT_SERVICE_PORT is not set, as per:

seldon-core/operator/controllers/seldondeployment_controller.go

Lines 680 to 687 in 2577801

	if !utils.HasEnvVar(con.Env, machinelearningv1.ENV_PREDICTIVE_UNIT_SERVICE_PORT) {
	con.Env = append(con.Env, []corev1.EnvVar{
	corev1.EnvVar{Name: machinelearningv1.ENV_PREDICTIVE_UNIT_SERVICE_PORT, Value: strconv.Itoa(int(portNum))},
	corev1.EnvVar{Name: machinelearningv1.ENV_PREDICTIVE_UNIT_ID, Value: con.Name},
	corev1.EnvVar{Name: machinelearningv1.ENV_PREDICTOR_ID, Value: p.Name},
	corev1.EnvVar{Name: machinelearningv1.ENV_SELDON_DEPLOYMENT_ID, Value: mlDep.ObjectMeta.Name},
	}...)
	}

These three environment variables would be useful to have within the container even when the port is not set, so would be good to move those out from the if statement.

However it would be important to understnad whether there is a historical reason why this was set, and whether removing it could cause issues.

[ukclivecox]

I imagine they are always set as this seems to be the only place that sets them. Do you have an example where they are not set?

However, its probably safe to always set them see I can't the user wanting to override them.

[axsaucedo]

Oh that's a good point, yeah you're right, if the user already set them we wouldn't want to override them. Yeah the example I have is the Streaming one, but the main reason is because I actually set PREDICTIVE_UNIT_SERVICE_PORT, however I still require these three variables to be set by the operator - is there a reason why these three variables are only set if the PORT env is not specificed? Otherwise it would be good to still set them (unless as you say it has been previously overridden)

[ukclivecox]

OK. I don't think we had a use case where the user sets PREDICTIVE_UNIT_SERVICE_PORT rather than the Operator doing it. I would suggest ensuring the Operator can't set the value itself - you can always set the port in the graph spec - see Tensorflow example in procotol_examples notebook.

I would suggest a change where we always set the other 3 envs and maybe check for the PREDICTIVE_UNIT_SERVICE_PORT if we think its really useful that a user could set it different from the port value in the graph - otherwise we should always set it.

[axsaucedo]

Sounds perfect, I think both suggestions make sense. Setting the other 3 env vars whilst still taking into account overrides would be useful. I actually didn't know you could set the port on the graph, but that makes more sense.

[axsaucedo]

One more thing to note (and perhaps something to discuss further) is that for the batch PR I had to also add another environment variable called PREDICTIVE_UNIT_SERVICE_HOST as it's necessary to provide the host to which the streaming service will be connecting. However now that you mention it it may make more sense to introduce a new environment variable as this would not be a service host, but more of a data source.