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.

Sign up for GitHub

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

Fix for k8s dict parsing #411

Merged
merged 5 commits into from
Nov 30, 2018
Merged

Fix for k8s dict parsing #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fb1b00a
Add support for minio artifacts
vanpelt Nov 27, 2018
32708db
Add new tests for parity
vanpelt Nov 27, 2018
643fa8e
Fix for sdk env bug
vanpelt Nov 29, 2018
e574ff6
Merge remote-tracking branch 'upstream/master'
vanpelt Nov 29, 2018
7f32049
improved test
vanpelt Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions sdk/python/kfp/compiler/compiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ def _convert_k8s_obj_to_dic(self, obj):
Returns: The serialized form of data.
"""

from six import text_type, integer_types
from six import text_type, integer_types, iteritems
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a style change? This function is copied directly from the official Kubernetes client library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the bug. I hit this issue because I tried to pass a dict into op.add_env_variable. Line 541 accesses iteritems but it was only defined in a conditional branch so it threw an "object accessed before assignment" error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another fix here may be to throw a ValueError if the env_variable isn't a k8s object, but It would be nice to be able to pass in a dict without having to use the k8s wrapper. Regardless, this is definitely a bug.

PRIMITIVE_TYPES = (float, bool, bytes, text_type) + integer_types
from datetime import date, datetime
if obj is None:
Expand All @@ -532,7 +532,6 @@ def _convert_k8s_obj_to_dic(self, obj):
# and attributes which value is not None.
# Convert attribute name to json key in
# model definition for request.
from six import iteritems
obj_dict = {obj.attribute_map[attr]: getattr(obj, attr)
for attr, _ in iteritems(obj.swagger_types)
if getattr(obj, attr) is not None}
Expand Down
16 changes: 16 additions & 0 deletions sdk/python/tests/compiler/compiler_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import tempfile
import unittest
import yaml
import datetime

class TestCompiler(unittest.TestCase):

Expand Down Expand Up @@ -141,6 +142,21 @@ def test_basic_workflow(self):
shutil.rmtree(tmpdir)
# print(tmpdir)

def test_convert_k8s_obj_to_dic_accepts_dict(self):
now = datetime.datetime.now()
converted = compiler.Compiler()._convert_k8s_obj_to_dic({
Copy link
Contributor

Choose a reason for hiding this comment

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

The argument given to _convert_k8s_obj_to_dic is not a k8s object. What exactly is this call trying to show?

Copy link
Contributor

Choose a reason for hiding this comment

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

_convert_k8s_obj_to_dic should not be an instance method. It's not using any compiler data.

"ENV": "test",
"number": 3,
"list": [1,2,3],
"time": now
})
self.assertEqual(converted, {
"ENV": "test",
"number": 3,
"list": [1,2,3],
"time": now.isoformat()
})

def test_composing_workflow(self):
"""Test compiling a simple workflow, and a bigger one composed from the simple one."""

Expand Down