Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for Kubernetes tolerations #1207
Add support for Kubernetes tolerations #1207
Changes from all commits
c58451e
c21ede9
8fc5812
a3c749f
af7aef8
d1c6f85
6f6dbee
bc39dde
165dcd3
b00048a
9cdb7ee
fa6e10a
fd9d1a4
b8eb851
401e93d
16a6734
d6992b2
53654e8
cb3b37d
d61d717
a1711b6
03e79c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is this needed anymore?
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.
Yes, it is. At this stage,
node_selector
is a tuple of strings.kubernetes.launch_job
expects a dictionaryThere 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.
Any reason not to handle this parsing within
kubernetes_job
- the actual format is dictated by the kubernetes SDK and that's why currently all the Kubernetes-related formatting is happening within the KubernetesJob object. As the SDK evolves, any changes would be isolated to that object.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.
node_selector
contains the value generated bywhich is a tuple of strings
kubernetes_job
expect a dictionary likeparse_node_selector
converts the tuple of strings to a dictionary compatible with the Kubernetes SDK.Does it make sense?
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.
I understand that the rationale for including this check in
_init_
is to ensure that this check is invoked forargo-workflows
too. However, this check will fail if the user hasn't installed the python packagekubernetes
yet - which is checked inpackage_init
- that check should technically happen before the check for tolerations.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.
It is checked in
kubernetes_cli
.The idea is that the check is invoked only if required, which means the python package
kubernetes
must be installed. I think the order of the execution doesn't matter.If
kubernetes
is not available,self.attributes["tolerations"]
is not being used, then the check is not required.Does it make sense to you?
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.
When the user pip installs metaflow, we don't install Kubernetes python package. It's only when the user starts executing a flow that involves
@kubernetes
or argo - we throw a nice warning asking them to install the python package. Now, if that first flow hastolerations
defined, then the user will instead get an error sayingno module named Kubernetes
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.
That import is inside a
try
block withIt should not raise any errors related to the missing module, is it correct?
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.
Yes - but it's the round about way this check is implemented which is my concern. We can ship this and come back to clean it up.