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

Add support for unicode string inputs to Workflow Transform in Triton #345

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add support for unicode string inputs to Workflow Transform in Triton #345

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
042a138
Add test for workflow with unicode string inputs
oliverholworthy May 10, 2023
980c1a5
Reformat test_ensemble.py
oliverholworthy May 10, 2023
926a6ce
Remove string dtype coercion
oliverholworthy May 10, 2023
02d25a7
Add string coercion to ensure we have unicode string array
oliverholworthy May 12, 2023
ddf4d36
Merge branch 'main' into workflow-with-unicode-string-inputs
oliverholworthy May 22, 2023
295b58b
Merge branch 'main' into workflow-with-unicode-string-inputs
karlhigley May 22, 2023
f8b17fa
Merge branch 'main' into workflow-with-unicode-string-inputs
oliverholworthy Jun 6, 2023
dcbe4da
Merge branch 'main' into workflow-with-unicode-string-inputs
oliverholworthy Jun 13, 2023
5644cd3
Merge branch 'main' into workflow-with-unicode-string-inputs
oliverholworthy Jun 14, 2023
a65aad2
Merge branch 'main' into workflow-with-unicode-string-inputs
karlhigley Jun 16, 2023
2c866a4
Use LambdaOp instead of Categorify in string test
oliverholworthy Jun 21, 2023
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: 0 additions & 3 deletions merlin/systems/triton/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,6 @@ def _convert_tensor(t):
out = t.as_numpy()
if len(out.shape) == 2:
out = out[:, 0]
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated to this change: It's unclear to me why we'd want to remove dimensions from the input here

Copy link
Contributor

@karlhigley karlhigley May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This code has existed for a long time, and I think is related to the perennial inconsistency around list formats that has plagued the Merlin code base. Way back in the before times, sometimes you'd get a proper 1d array/tensor and sometimes you'd get a 2d array/tensor that only contained one row. The legacy serving code from NVT that Systems is based on (and still trying to clean up and/or shed) had all kinds of issues like this and mostly solved them by hacking around the inconsistent formats instead of standardizing.

# cudf doesn't seem to handle dtypes like |S15 or object that well
if is_string_dtype(out.dtype):
out = out.astype("str")
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried changing this to out = np.char.decode(out.astype(bytes)) which worked for the new test being added here. And then I wondered if this was needed at all. Looking to see if the tests pass without this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

My impression is that this does (or did) cover a real edge case, which may not be adequately covered by tests. This piece of code was inherited from the old serving code in NVT, which was TBH not very well tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this keeing the string type coercion. Using np.char.decode(out.astype(bytes)) intead of out.astype("str").

It appears we do need this because cudf doesn't accept an array of byte strings as a type when constructing a DataFrame.

return out


Expand Down
30 changes: 30 additions & 0 deletions tests/unit/systems/dag/runtimes/triton/ops/workflow/test_ensemble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,3 +366,33 @@ def test_workflow_dtypes(tmpdir):
)
for key, value in expected_response.items():
np.testing.assert_array_equal(response[key], value)


@pytest.mark.skipif(not TRITON_SERVER_PATH, reason="triton server not found")
def test_workflow_with_string_input(tmpdir):
"""This test checks that we can pass strings with unicode characters to a workflow in Triton."""
df = make_df({"a": ["椅子", "καρέκλα", "כִּסֵא", "chair"]})
dataset = Dataset(df)
workflow_ops = ["a"] >> wf_ops.Categorify()
workflow = Workflow(workflow_ops)
workflow.fit(dataset)

workflow_node = workflow.input_schema.column_names >> workflow_op.TransformWorkflow(workflow)
wkflow_ensemble = ensemble.Ensemble(workflow_node, workflow.input_schema)
ensemble_config, node_configs = wkflow_ensemble.export(tmpdir)

with run_triton_server(tmpdir) as client:
for model_name in [ensemble_config.name, node_configs[0].name]:
request_dict = {
"a": np.array(["椅子", "καρέκλα", "כִּסֵא", "chair"], dtype="object"),
}
expected_response = {
"a": np.array([1, 2, 3, 4], dtype="int32"),
}
schema = workflow.input_schema
input_table = TensorTable(request_dict)
output_names = ["a"]
response = send_triton_request(
schema, input_table, output_names, client=client, triton_model=model_name
)
assert set(expected_response["a"].tolist()) == set(response["a"].tolist())