Pipelines: kubeflow_tfjob_launcher_op to support volumes

[ryandawsonuk]

I want to be able to load a TFJob with a mounted directory so that the output can be written and used in the next step. Much of the job template is configurable as the template is loaded as a dict and modified using the operation parameters. But this doesn't work so neatly for volumes as they also need volume mounts and rather than just replacement we need the option for whole sections to either be there or not.

To accommodate volumes it might work better to inline the whole file definition as a multi-line string. Then sections could be added into the composition of the string. If there are no volumes passed in (ideally using pvolumes) then the section for volumes would be an empty string. This is just a suggestion - perhaps there are better templating approaches available.

[elikatsis]

/assign @elikatsis

Let's pick it from where we left in #1345.

1. https://github.com/kubeflow/pipelines/issues/1345#issuecomment-493138678
2. https://github.com/kubeflow/pipelines/issues/1345#issuecomment-493142939

The goal is to determine what should the API cover for a TFJobOp.

In my opinion, the constructor's arguments shouldn't cover the whole TFJob spec. Since TFJobOp is a subclass of ResourceOp, it is always possible to specify a TFJob via a dictionary or a class (using some sort of client if one exists) and pass it as k8s_resource.

The arguments should make the common case fast, exposing those specs which have to be specified most of the times, following the VolumeOp class as an example.

[ryandawsonuk]

I was thinking the same parameters as the existing TFJob launcher that works via ContianerOp. There is a comment referencing args but would make sense to use the args in kubeflow_tfjob_launcher_op.py as those seem to be the ones that are used. Extra arguments would be needed for volumes. Effectively we need to add the pvc-claim name and volume mount path (and maybe the volume name? perhaps that could be set to something arbitrary?). So could maybe be like other uses of pvolumes.

I wonder if the best way to handle the conditionals options in generating sections from a template would be some basic jinja2 rather than plain python code with if statements. There are some examples out there and it would provide the flexibility to add environment variables only if needed and mount volumes only if the user wants them and could be expanded in the future. It might be tempting to think of kubectl for templating but in extending ResourceOp we presumably don't have access to that as it would be a tool that would run in a container.

Presumably extending ResourceOp and handling the TFJob as an argo resource step would allow for dependency on the created resource to be captured by hooking into argo's native successCondition to wait for the TFJob to complete - as seen in the argo project's own ML example.

[ryandawsonuk]

I have been thinking about whether kustomize could be used to provide the variations on a TFJob instead of jina2. But it's not easy to see how to do that as kustomize works on different principles. Python built-in string functions is another way that could be sufficient for this purpose.

[hougangliu]

/assign

[hougangliu]

/unassign

[ryandawsonuk]

I notice support for volumes is being added to tfjob_launcher_op in https://github.com/kubeflow/pipelines/pull/1494

Looks like it expects volumes to be specified in a dict of k-v pairs for pvc-name (key) and mountPath (value). Is that right @hougangliu ?

Am wondering how it could be used with pvolumes. I guess you could get the name of the pvolume and construct a dict.

Am not saying it has to work with pvolumes directly. Just wondering how you would do it.

[elikatsis]

@ryandawsonuk
Hey Ryan,

I have added a comment on that PR proposing some refactoring.
If that is OK with @hougangliu, then it seems to me that PipelineVolumes will be supported seamlessly.

[stale[bot]]

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.

[stale[bot]]

This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.