Skip to content

Commit

Permalink
SDK - Components - Improved stability of the input and output renaming (
Browse files Browse the repository at this point in the history
#3738)

In some cases the input and output names need to be converted (for example, the input names need to be converted to python function parameter names).
With naive renaming, multiple inputs might be mapped to the same parameter name in some edge cases. The `generate_unique_name_conversion_table` creates a correct mapping.

However, in some really rare cases the resulting mapping could be confusing since it might rename an input whose name was already a correct parameter name and map a different input name to that parameter. E.g. {'AAA' -> 'aaa', 'aaa' -> 'aaa_2'}.
This PR fixes that. Names that do not change when applying the conversion_func will remain unchanged in the mapping. {'AAA' -> 'aaa_2', 'aaa' -> 'aaa'}.
  • Loading branch information
Ark-kun authored May 13, 2020
1 parent 5435e87 commit d418f57
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 0 deletions.
10 changes: 10 additions & 0 deletions sdk/python/kfp/components/_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,17 @@ def generate_unique_name_conversion_table(names: Sequence[str], conversion_func:
'''
forward_map = {}
reverse_map = {}

# Names that do not change when applying the conversion_func should remain unchanged in the table
names_that_need_conversion = []
for name in names:
if conversion_func(name) == name:
forward_map[name] = name
reverse_map[name] = name
else:
names_that_need_conversion.append(name)

for name in names_that_need_conversion:
if name in forward_map:
raise ValueError('Original name {} is not unique.'.format(name))
converted_name = _convert_name_and_make_it_unique_by_adding_number(name, reverse_map, conversion_func)
Expand Down
30 changes: 30 additions & 0 deletions sdk/python/tests/components/test_components.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,36 @@ def test_handle_similar_input_names(self):
'''
task_factory1 = comp.load_component_from_text(component_text)

def test_conflicting_name_renaming_stability(self):
# Checking that already pythonic input names are not renamed
# Checking that renaming is deterministic
component_text = textwrap.dedent('''\
inputs:
- {name: Input 1}
- {name: Input_1}
- {name: Input-1}
- {name: input_1} # Last in the list, but is pythonic, so it should not be renamed
implementation:
container:
image: busybox
command:
- inputValue: Input 1
- inputValue: Input_1
- inputValue: Input-1
- inputValue: input_1
'''
)
task_factory1 = comp.load_component(text=component_text)
task1 = task_factory1(
input_1_2='value_1_2',
input_1_3='value_1_3',
input_1_4='value_1_4',
input_1='value_1', # Expecting this input not to be renamed
)
resolved_cmd = _resolve_command_line_and_paths(task1.component_ref.spec, task1.arguments)

self.assertEqual(resolved_cmd.command, ['value_1_2', 'value_1_3', 'value_1_4', 'value_1'])

def test_handle_duplicate_input_output_names(self):
component_text = '''\
inputs:
Expand Down

0 comments on commit d418f57

Please sign in to comment.