-
Notifications
You must be signed in to change notification settings - Fork 310
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
Do not import kubernetes in flytekit core #3118
base: master
Are you sure you want to change the base?
Do not import kubernetes in flytekit core #3118
Conversation
Signed-off-by: Thomas J. Fan <[email protected]>
Code Review Agent Run #6680ecActionable Suggestions - 0Additional Suggestions - 1
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
what do you think of adding an extra called |
It's either |
agreed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove kubernetes>=12.0.1
in pyproject.toml
?
https://github.com/flyteorg/flytekit/blob/master/pyproject.toml#L36
Ah I see. I did not know |
wait, we can still remove it, right? Adding a 2MB dependency that doesn't get used all the time seems like a waste. |
another great argument for removing kubernetes as a dependency (again) is because it wasn't put back in an official release yet (as per 2da64ef - only 1.15 beta releases). |
Signed-off-by: Thomas J. Fan <[email protected]>
Updated PR to remove |
flytekit/core/task.py
Outdated
@@ -211,6 +211,7 @@ def task( | |||
pod_template_name: Optional[str] = None, | |||
accelerator: Optional[BaseAccelerator] = None, | |||
pickle_untyped: bool = False, | |||
new_kwargs: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a leftover from a test you were running, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I forgot to remove this. I reverted the change.
Code Review Agent Run #860995Actionable Suggestions - 0Review Details
|
Signed-off-by: Thomas J. Fan <[email protected]>
…t_import_kubernetes_v2 Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3118 +/- ##
==========================================
- Coverage 76.08% 76.06% -0.03%
==========================================
Files 203 203
Lines 21609 21609
Branches 2782 2782
==========================================
- Hits 16442 16437 -5
- Misses 4346 4349 +3
- Partials 821 823 +2 ☔ View full report in Codecov by Sentry. |
Code Review Agent Run #6dbf04Actionable Suggestions - 0Review Details
|
Can we just make sure we get a good error if it is not installed and needed at compile time. We can ask folks to install it |
Tracking issue
Follow up to #2943
Why are the changes needed?
Since flytekit does not depend on
kubernetes
, importing flytekit will not work anymore ifkubernetes
is not installed.What changes were proposed in this pull request?
This PR moves the imports out of the top level and into the function.
How was this patch tested?
Built an image with this PR and not kubernetes and make sure it works:
Ideally we also have a CI job that does not have kubernetes installed to make sure flytekit still works.
Summary by Bito
Optimization of kubernetes dependency handling in Flytekit by making it optional and lightweight, while also enhancing the authentication system through factory pattern implementation and lazy loading capabilities. The changes include moving kubernetes-related imports to function scope, implementing environment variable validation in Secret class, and improving credential management with better refresh mechanisms.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2