Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proto-docs: Compile-time and Runtime PodTemplates #3391

Merged
merged 7 commits into from
Jun 2, 2023

Conversation

eapolinario
Copy link
Contributor

@eapolinario eapolinario commented Mar 3, 2023

Tracking issue

fixes #3385

Describe your changes

Documentation for Compile-time and Runtime PodTemplates feature.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Note to reviewers

Signed-off-by: Eduardo Apolinario <[email protected]>
@hamersaw
Copy link
Contributor

hamersaw commented Mar 6, 2023

A few thoughts here:
(1) Should we be thinking of this as statically vs dynamically set rather than client-side vs server-side? In the realm of Flyte on k8s I think client / server boundaries are a little fuzzy, IMO a compile-time vs runtime differentiation is a little more understandable but maybe I'm too close to this.
(2) Is there anyway we can improve description on the order of application in this PR? With the original PodTemplate work that was a big concern that we had many issues on.

nit: stick to uniformity on PodTemplate terminology in different sentences we're using pod template or podtemplate.

@cosmicBboy
Copy link
Contributor

Weird, sphinx thinks the subheader hierarchy is off https://github.com/flyteorg/flyte/actions/runs/4320063811/jobs/7539915673#step:5:121

Can you try using ^^^^^^^^^ instead of ------------?

Also, we should also add a new example in flytesnacks under the Kubernetes Pod integration
https://docs.flyte.org/projects/cookbook/en/latest/auto/integrations/kubernetes/pod/index.html

@hamersaw
Copy link
Contributor

hamersaw commented Mar 7, 2023

under the Kubernetes Pod integration

that's not the correct place as these will apply everywhere - ContainerTask, ShellTask, Dask / Ray plugins, etc

@cosmicBboy
Copy link
Contributor

under the Kubernetes Pod integration

that's not the correct place as these will apply everywhere - ContainerTask, ShellTask, Dask / Ray plugins, etc

In that case, thoughts on where this should go?

Signed-off-by: eduardo apolinario <[email protected]>
@eapolinario eapolinario changed the title Proto-docs: client-side pod templates Proto-docs: Compile-time and Runtime PodTemplates Jun 1, 2023
@eapolinario
Copy link
Contributor Author

under the Kubernetes Pod integration

that's not the correct place as these will apply everywhere - ContainerTask, ShellTask, Dask / Ray plugins, etc

In that case, thoughts on where this should go?

How about we add those in each relevant section? For example:

@cosmicBboy , what do you think?

@eapolinario
Copy link
Contributor Author

eapolinario commented Jun 1, 2023

Here's how the little graph renders in the docs:
Screenshot 2023-06-01 at 10 17 48 AM


If you have the default PodTemplate defined in the ``flyte`` namespace
Example 1: Runtime PodTemplate and K8s Plugin Configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of these examples may be redundant. Example 3 covers Example 1 and Example 2 right? OK to leave all as well.

Copy link
Contributor Author

@eapolinario eapolinario Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just want to make it clear that we can mix and match (up to a point obviously, since one cannot use a default Runtime PodTemplate and a named one at the same time).


Set the ``default-pod-template-name`` in FlytePropeller
========================================================
--------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Set the default-pod-template-name in FlytePropeller above this line is only required when using a default PodTemplate and not if it is set in the task decorator. Does it make sense to make this distinction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to add a little blurb explaining the difference between the default Runtime PodTemplate and a named one.


The term compile-time here refers to the fact that the pod template definition is part of the `TaskSpec <https://docs.flyte.org/projects/flyteidl/en/latest/protos/docs/admin/admin.html#ref-flyteidl-admin-taskclosure>`__.

Runtime PodTemplate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explicitly say that the default-pod-template-name is used unless it is overridden by the pod-template-name in the task decorator. Only one may be applied. Not sure where is the best fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

hamersaw
hamersaw previously approved these changes Jun 2, 2023
Copy link
Contributor

@hamersaw hamersaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to read in case (1) is active and FlytePropeller is building a Pod where (2) was used as part of the definition of the task, then only the values of the PodTemplate mentioned in (2) will be used to build the Pod. multiple times to understand. Might consider rewording? If not, OK too.

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: eduardo apolinario <[email protected]>
@eapolinario eapolinario merged commit 14171fd into master Jun 2, 2023
@eapolinario eapolinario deleted the add-task-podtemplate-docs branch June 2, 2023 17:04
npapapietro pushed a commit to npapapietro/flyte that referenced this pull request Jun 6, 2023
* Proto-docs: client-side pod templates

Signed-off-by: Eduardo Apolinario <[email protected]>

* In between change

Signed-off-by: eduardo apolinario <[email protected]>

* Compile-time versus Runtime PodTemplates

Signed-off-by: eduardo apolinario <[email protected]>

* Change top-level heading to match https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections

Signed-off-by: eduardo apolinario <[email protected]>

* Explain the difference between the two types of Runtime PodTemplates

Signed-off-by: eduardo apolinario <[email protected]>

* Disambiguate the evaluation precedence of Runtime PodTemplates.

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: eduardo apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: eduardo apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Nathan Papapietro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] Add PodTemplate documentation: deployment configuration docs update and an example in flytesnacks
3 participants