Don't overwrite PYTHONUNBUFFERED in PyTorchJob (and others)

[brannondorsey]

The PyTorch controller sets PYTHONUNBUFFERED=0 below, independent of wether or not the user has configured this variable themselves.

training-operator/pkg/controller.v1/pytorch/envvar.go

Lines 54 to 60 in bb2b58a

	// Set PYTHONUNBUFFERED to true, to disable output buffering.
	// Ref https://stackoverflow.com/questions/59812009/what-is-the-use-of-pythonunbuffered-in-docker-file.
	podTemplateSpec.Spec.Containers[i].Env = append(
	podTemplateSpec.Spec.Containers[i].Env, corev1.EnvVar{
	Name: "PYTHONUNBUFFERED",
	Value: "1",
	})

When I set this value myself in a PyTorchJob as shown below, the resulting pod created by the controller has a second PYTHONUNBUFFERED environment variable set by the controller, which overwrites the user-defined value.

apiVersion: "kubeflow.org/v1"
kind: PyTorchJob
metadata:
  name: example
spec:
  pytorchReplicaSpecs:
    Master:
      replicas: 1
      template:
        spec:
          containers:
            - name: pytorch
              image: ubuntu
              env:
                - name: PYTHONUNBUFFERED
                  value: "1"
              #...

# output snippet from `kubectl describe pod <master|worker>`
    Environment:
      PYTHONUNBUFFERED:             1
      PYTHONUNBUFFERED:             0

The behavior that I would expect is that the PyTorch job controller sets PYTHONUNBUFFERED if and only if the user has not already set it in their own env definition.

Based on a quick git grep, I also expect this behavior to be occurring in the PaddleJob and XGBoostJob controllers as well.

NOTE: The value of this variable set in master is now 0 instead of 1 (see PR), however my code snippet is from a controller with version v1.6.0, where it is still set to 0. Even though v1.7.0 will fix my use case, I'd argue this is something which shouldn't be overwritten by the controller, if the user defines the environment variable in the pod spec themselves.

[tenzen-y]

It makes sense.
/kind feature

[tenzen-y]

Feel free to open PR.

[sxwl-donggang]

If users can config env "PYTHONUNBUFFERED"，they can also config other env like "WORLD_SIZE"、“RANK”. This is a common problem. Whether all the env setting place should keep the user configured value.

[tenzen-y]

If users can config env "PYTHONUNBUFFERED"，they can also config other env like "WORLD_SIZE"、“RANK”. This is a common problem. Whether all the env setting place should keep the user configured value.

I don't think that we should allow users to modify the WORLD_SIZE and RANK since those envs are managed by the training-operator.
If users wan to modify those envs, users need not to use the training-operator. The batch/v1 job should be enough.

[sxwl-donggang]

If users can config env "PYTHONUNBUFFERED"，they can also config other env like "WORLD_SIZE"、“RANK”. This is a common problem. Whether all the env setting place should keep the user configured value.

I don't think that we should allow users to modify the WORLD_SIZE and RANK since those envs are managed by the training-operator. If users wan to modify those envs, users need not to use the training-operator. The batch/v1 job should be enough.

So training-operator setPodEnv function should overwrite the WORLD_SIZE and RANK if user set instead of append .

			podTemplateSpec.Spec.Containers[i].Env = append(podTemplateSpec.Spec.Containers[i].Env, corev1.EnvVar{
				Name:  "WORLD_SIZE",
				Value: strconv.Itoa(worldSize),
			})
			podTemplateSpec.Spec.Containers[i].Env = append(podTemplateSpec.Spec.Containers[i].Env, corev1.EnvVar{
				Name:  "RANK",
				Value: strconv.Itoa(rank),
			})

[tenzen-y]

If users can config env "PYTHONUNBUFFERED"，they can also config other env like "WORLD_SIZE"、“RANK”. This is a common problem. Whether all the env setting place should keep the user configured value.

I don't think that we should allow users to modify the WORLD_SIZE and RANK since those envs are managed by the training-operator. If users wan to modify those envs, users need not to use the training-operator. The batch/v1 job should be enough.

So training-operator setPodEnv function should overwrite the WORLD_SIZE and RANK if user set instead of append .

			podTemplateSpec.Spec.Containers[i].Env = append(podTemplateSpec.Spec.Containers[i].Env, corev1.EnvVar{
				Name:  "WORLD_SIZE",
				Value: strconv.Itoa(worldSize),
			})
			podTemplateSpec.Spec.Containers[i].Env = append(podTemplateSpec.Spec.Containers[i].Env, corev1.EnvVar{
				Name:  "RANK",
				Value: strconv.Itoa(rank),
			})

Or, we can add validations to reject Jobs with managed envs like RANK.

[sxwl-donggang]

If users can config env "PYTHONUNBUFFERED"，they can also config other env like "WORLD_SIZE"、“RANK”. This is a common problem. Whether all the env setting place should keep the user configured value.

I don't think that we should allow users to modify the WORLD_SIZE and RANK since those envs are managed by the training-operator. If users wan to modify those envs, users need not to use the training-operator. The batch/v1 job should be enough.

So training-operator setPodEnv function should overwrite the WORLD_SIZE and RANK if user set instead of append .

			podTemplateSpec.Spec.Containers[i].Env = append(podTemplateSpec.Spec.Containers[i].Env, corev1.EnvVar{
				Name:  "WORLD_SIZE",
				Value: strconv.Itoa(worldSize),
			})
			podTemplateSpec.Spec.Containers[i].Env = append(podTemplateSpec.Spec.Containers[i].Env, corev1.EnvVar{
				Name:  "RANK",
				Value: strconv.Itoa(rank),
			})

Or, we can add validations to reject Jobs with managed envs like RANK.

Yep! Using webhooks to validate managed environment variables should work.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tenzen-y]

/remove-lifecycle stale

[tariq-hasan]

@tenzen-y I am interested to take on this ticket.

[tenzen-y]

@tenzen-y I am interested to take on this ticket.

@tariq-hasan Sorry, this depends on some other issues. So, this is not a good first issue...
/remove-help
/remove-good-first-issue

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tenzen-y]

/remove lifecycle-stale

[tenzen-y]

/remove-lifecycle stale

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tenzen-y]

/remove-lifecycle stale

@tariq-hasan Hi, this was unblocked by #1993.
So, if you are still interested in this issue, it would be great.

[tariq-hasan]

Sounds good. Thank you so much.

/assign