fix: Defer kubernetes imports to method level for use with local mode#3167
Conversation
|
🎉 Welcome to the Kubeflow Trainer! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
Pull Request Test Coverage Report for Build 21711780024Details
💛 - Coveralls |
Signed-off-by: Fiona-Waters <[email protected]>
Fiona-Waters
force-pushed
the
move-imports
branch
from
28a3605 to
c54e72c
Compare
|
@andreyvelich @astefanutti please take a look. Thank you. |
| from kubernetes import client, config | ||
| from kubernetes.client.rest import ApiException | ||
| from kubernetes.dynamic.exceptions import ConflictError | ||
|
|
There was a problem hiding this comment.
Interesting, so we plan to re-use the same initializers for container execution as well?
How are we planning to use cache initializer there?
cc @akshaychitneni
There was a problem hiding this comment.
Only data and model initializers are supported for container execution. Since container backend is designed for simpler local execution I don't think it makes sense to support cache initializer there. WDYT?
There was a problem hiding this comment.
Correct, we are not going to support data cache initializer in container backed for now: kubeflow/sdk#188 (comment)
In that case, why do we need to move these imports under def download_dataset(self): ?
There was a problem hiding this comment.
It's because of this import - https://github.com/kubeflow/trainer/blob/master/pkg/initializers/dataset/__main__.py#L6
maybe it would be better to move that inside the relevant case instead?
There was a problem hiding this comment.
That'd make sense to move the imports in each corresponding case.
There was a problem hiding this comment.
I've updated to do this. Thanks. I don't think that the PR checks that are failing are related to my changes - could we re-run the tests?
|
Thanks @Fiona-Waters! /lgtm |
Signed-off-by: Fiona-Waters <[email protected]>
Signed-off-by: Fiona-Waters <[email protected]>
Fiona-Waters
force-pushed
the
move-imports
branch
from
d19cf1c to
233e088
Compare
|
/retest |
|
/lgtm Thanks @Fiona-Waters! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: astefanutti The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What this PR does / why we need it:
Following on from this Kubeflow SDK PR adding Initializers to local podman/docker mode, these imports need to be deffered to the method level so that kubernetes config is not required in these local environments, these changes do not break existing functionality.
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #
Checklist: