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

Adding missing fields to FlyteTask remote entity #3093

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

UmerAhmad
Copy link

@UmerAhmad UmerAhmad commented Jan 25, 2025

Tracking issue

flyteorg/flyte#6192

Why are the changes needed?

Missing fields from FlyteTask entity from remote, for example, we need k8s_pod for template data analyis, but it only includes container as of now.

What changes were proposed in this pull request?

Adding the following fields to FlyteTask init, and promotion from model (also config):

security_context
config
k8s_pod
sql
extended_resources

How was this patch tested?

unit tests

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

Summary by Bito

The enhanced FlyteTask remote entity now includes additional fields: security_context, k8s_pod, sql, and extended_resources. The implementation adds proper initialization of SQL and Kubernetes pod template fields with default None values, includes parameter validation for data integrity, and implements a new property method for accessing extended_resources.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 1

Copy link

welcome bot commented Jan 25, 2025

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 25, 2025

Code Review Agent Run #42cb06

Actionable Suggestions - 1
  • flytekit/remote/entities.py - 1
    • Consider adding parameter validation checks · Line 52-56
Review Details
  • Files reviewed - 1 · Commit Range: 70784e4..70784e4
    • flytekit/remote/entities.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@UmerAhmad UmerAhmad force-pushed the add-missing-fields-to-flytetask-entity branch from 70784e4 to ef01024 Compare January 25, 2025 08:48
@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 25, 2025

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Enhanced FlyteTask Entity with Additional Configuration Fields

entities.py - Added support for security_context, k8s_pod, sql, and extended_resources fields in FlyteTask

test_remote.py - Updated test cases to handle new fields in FlyteTask entity

Comment on lines +52 to +56
security_context=None,
config=None,
k8s_pod=None,
sql=None,
extended_resources=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding parameter validation checks

Consider adding parameter validation for the new parameters security_context, k8s_pod, sql, and extended_resources to ensure they contain valid values before being passed to the task template.

Code suggestion
Check the AI-generated fix before applying
 @@ -51,6 +51,14 @@
          task_type_version: int = 0,
          security_context=None,
          config=None,
          k8s_pod=None,
          sql=None,
          extended_resources=None,
 +        if security_context is not None and not isinstance(security_context, dict):
 +            raise ValueError('security_context must be a dict or None')
 +        if k8s_pod is not None and not isinstance(k8s_pod, dict):
 +            raise ValueError('k8s_pod must be a dict or None')
 +        if sql is not None and not isinstance(sql, dict):
 +            raise ValueError('sql must be a dict or None')
 +        if extended_resources is not None and not isinstance(extended_resources, dict):
 +            raise ValueError('extended_resources must be a dict or None')

Code Review Run #42cb06


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

@UmerAhmad UmerAhmad changed the title Adding missing fields to flytetask remote entity Adding missing fields to FlyteTask remote entity Jan 25, 2025
@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 25, 2025

Code Review Agent Run #c16628

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: ef01024..ef01024
    • flytekit/remote/entities.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 26, 2025

Code Review Agent Run #4cb82d

Actionable Suggestions - 0
Additional Suggestions - 1
  • tests/flytekit/unit/remote/test_remote.py - 1
    • Consider combining similar None assignments · Line 832-833
Review Details
  • Files reviewed - 1 · Commit Range: ef01024..8f696e6
    • tests/flytekit/unit/remote/test_remote.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

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.

2 participants