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

pod template inplace operations #2899

Merged

Conversation

dansola
Copy link
Contributor

@dansola dansola commented Nov 4, 2024

Why are the changes needed?

When a constant is used for a pod template, subsequent uses of that pod template will contain information from other tasks that use the same pod template and will overwrite information in the task decorator. Mutation of the pod template object the user passes happens here:

cast(V1PodSpec, pod_template.pod_spec).containers = final_containers

What changes were proposed in this pull request?

In _serialize_pod_spec we use a deep copy of the pod template rather than making in place operations on the pod template that the user passes.

How was this patch tested?

import flytekit as fk

POD_TEMPLATE = fk.PodTemplate(
    labels={"lKeyA": "lValA", "lKeyB": "lValB"},
)

image_x = fk.ImageSpec(
    registry="ghcr.io/dansola",
    name="test-image",
    packages=["sqlalchemy", "kubernetes"]
)

image_y = fk.ImageSpec(
    registry="ghcr.io/dansola",
    name="test-image",
    packages=["pandas", "kubernetes"]
)

@fk.task(container_image=image_x, pod_template=POD_TEMPLATE)
def do_x():
    import sqlalchemy
    print(sqlalchemy.__version__)


@fk.task(container_image=image_y, pod_template=POD_TEMPLATE)
def do_y():
    import pandas
    print(pandas.__version__)

@fk.workflow
def wf():
    x = do_x()
    y = do_y()

    x >> y

Right now the above workflow fails because do_y uses image_x and does not have pandas.

After the deep copy, do_y actually uses image_y.

Check all the applicable boxes

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

Related PRs

Docs link

Signed-off-by: Daniel Sola <daniel.sola@union.ai>
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Can you add a test to make sure that the pod template does not get mutated?

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 40.31%. Comparing base (0662bc6) to head (f92382a).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
flytekit/core/utils.py 50.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (0662bc6) and HEAD (f92382a). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (0662bc6) HEAD (f92382a)
3 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2899       +/-   ##
===========================================
- Coverage   75.65%   40.31%   -35.34%     
===========================================
  Files         199      199               
  Lines       20765    20708       -57     
  Branches     2665     2665               
===========================================
- Hits        15710     8349     -7361     
- Misses       4293    12258     +7965     
+ Partials      762      101      -661     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dansola
Copy link
Contributor Author

dansola commented Nov 4, 2024

Can you add a test to make sure that the pod template does not get mutated?

Should I write the test on PythonAutoContainerTask or _serialize_pod_spec or something more end-to-end? Not exactly sure the best way to test FlyteRemote calls which is what triggers this.

@thomasjpfan
Copy link
Member

Should I write the test on PythonAutoContainerTask

For simplicity, I would call PythonAutoContainerTask.get_k8s_pod and make sure that the pod template does not change.

@dansola
Copy link
Contributor Author

dansola commented Nov 4, 2024

Should I write the test on PythonAutoContainerTask

For simplicity, I would call PythonAutoContainerTask.get_k8s_pod and make sure that the pod template does not change.

Might need some help from the OSS team on the tests. I'm running into some errors that look like:

╭───────────────────── Traceback (most recent call last) ──────────────────────╮
│ /Users/danielsola/repos/wine-classification/pi_pod_templates/unit_test_draft │
│ .py:28 in <module>                                                           │
│                                                                              │
│ ❱ 28 pod = python_auto_container_task.get_k8s_pod(settings=settings)         │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/python_auto_container.py:238 in get_k8s_pod             │
│                                                                              │
│ ❱ 238 │   │   │   pod_spec=_serialize_pod_spec(self.pod_template, self._get_ │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/python_auto_container.py:220 in _get_container          │
│                                                                              │
│ ❱ 220 │   │   │   args=self.get_command(settings=settings),                  │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/python_auto_container.py:196 in get_command             │
│                                                                              │
│ ❱ 196 │   │   return self._get_command_fn(settings)                          │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/python_auto_container.py:164 in get_default_command     │
│                                                                              │
│ ❱ 164 │   │   │   *self.task_resolver.loader_args(settings, self),           │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/python_auto_container.py:279 in loader_args             │
│                                                                              │
│ ❱ 279 │   │   _, m, t, _ = extract_task_module(task)                         │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/tracker.py:354 in extract_task_module                   │
│                                                                              │
│ ❱ 354 │   │   │   name = f.lhs                                               │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/tracker.py:120 in lhs                                   │
│                                                                              │
│ ❱ 120 │   │   return self.find_lhs()                                         │
│                                                                              │
│ /Users/danielsola/miniconda3/envs/wine-classification/lib/python3.11/site-pa │
│ ckages/flytekit/core/tracker.py:172 in find_lhs                              │
│                                                                              │
│ ❱ 172 │   │   raise _system_exceptions.FlyteSystemException(f"Error looking  │
╰──────────────────────────────────────────────────────────────────────────────╯
FlyteSystemException: SYSTEM:Unknown: error=Error looking for LHS in 
unit_test_draft

@wild-endeavor
Copy link
Contributor

@dansola let's chat tomorrow.

Signed-off-by: Daniel Sola <daniel.sola@union.ai>
@dansola
Copy link
Contributor Author

dansola commented Nov 5, 2024

Should I write the test on PythonAutoContainerTask

For simplicity, I would call PythonAutoContainerTask.get_k8s_pod and make sure that the pod template does not change.

@thomasjpfan I pushed a test!

@wild-endeavor
Copy link
Contributor

re-ran the failed test.

@wild-endeavor wild-endeavor enabled auto-merge (squash) November 6, 2024 17:49
@wild-endeavor wild-endeavor merged commit 99b6e97 into master Nov 6, 2024
102 of 104 checks passed
kumare3 pushed a commit that referenced this pull request Nov 8, 2024
Signed-off-by: Daniel Sola <daniel.sola@union.ai>
katrogan pushed a commit that referenced this pull request Nov 15, 2024
Signed-off-by: Daniel Sola <daniel.sola@union.ai>
Signed-off-by: Katrina Rogan <katroganGH@gmail.com>
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.

3 participants