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.

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

Script annotations + runner for annotated strings breaks for json strs #855

Closed
elliotgunton opened this issue Nov 9, 2023 · 1 comment · Fixed by #859
Closed

Script annotations + runner for annotated strings breaks for json strs #855

elliotgunton opened this issue Nov 9, 2023 · 1 comment · Fixed by #859
Labels
type:bug A general bug

Comments

@elliotgunton
Copy link
Collaborator

          Discussed with @samj1912 - the runner currently does some implicit json loading of inputs that we'd like to review/remove some of and move the control of loading parameters (e.g. given a json str or yaml str, choose whether to load it via `json.loads` or `yaml.safe_load`). Marked the PR as draft, and will make the change in another PR on top of this one.

Also, this code doesn't deal well with Annotated strings, resulting in different behaviour for my_str: str and my_str: Annotated[str, Parameter(...)] in script parameters

type_ = inspect.signature(f).parameters[key].annotation

Originally posted by @elliotgunton in #850 (comment)

@elliotgunton elliotgunton changed the title Script annotations + runner for strings Script annotations + runner for annotated strings breaks for json strs Nov 9, 2023
@elliotgunton
Copy link
Collaborator Author

We can show the problem with this workflow. The CustomScriptConstructor just wraps the runner script constructor to print the incoming kwargs list for sanity checks (i.e. it does receive a json str)

import json
from typing import Annotated

from hera.workflows import Parameter, Steps, WorkflowTemplate, script
from testrunnerloading.custom_runner import CustomScriptConstructor

@script(constructor=CustomScriptConstructor())
def take_json_str(my_json: Annotated[str, Parameter(name="my-json")]):
    """Take a json string, load it and print it."""
    print(json.loads(my_json))


def get_workflow_template() -> WorkflowTemplate:
    """Create and return the test-runner-loading WorkflowTemplate."""
with WorkflowTemplate(
    name="mlplatform.test-runner-loading",
    entrypoint="steps",
) as wt:
    with Steps(name="steps"):
        take_json_str(arguments={"my_json": json.dumps({"key": "value"})})

Which gets an error like:

source contents: [{"name":"my_json","value":"{\"key\": \"value\"}"}]
kwargs_list: [{'name': 'my_json', 'value': '{"key": "value"}'}]
calling function
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/workspace/testrunnerloading/custom_runner.py", line 350, in <module>
    _run()
  File "/workspace/testrunnerloading/custom_runner.py", line 343, in _run
    result = _runner(args.entrypoint, kwargs_list)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/testrunnerloading/custom_runner.py", line 315, in _runner
    return function(**kwargs)
           ^^^^^^^^^^^^^^^^^^
  File "/workspace/testrunnerloading/custom_runner.py", line 58, in inner
    return f(**filtered_kwargs)
           ^^^^^^^^^^^^^^^^^^^^
  File "pydantic/decorator.py", line 40, in pydantic.decorator.validate_arguments.validate.wrapper_function
  File "pydantic/decorator.py", line 133, in pydantic.decorator.ValidatedFunction.call
  File "pydantic/decorator.py", line 130, in pydantic.decorator.ValidatedFunction.init_model_instance
  File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for TakeJsonStr
my_json
  str type expected (type=type_error.str)
time="2023-11-09T11:53:38 UTC" level=info msg="sub-process exited" argo=true error="<nil>"
Error: exit status 1

@elliotgunton elliotgunton added the type:bug A general bug label Nov 9, 2023
elliotgunton added a commit that referenced this issue Nov 23, 2023
**Pull Request Checklist**
- [x] Fixes #855 
- [x] Tests added
- [x] Documentation/examples added
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
Currently, annotations break the `_is_str_kwarg_of` function. Adds logic
to check whether it's a built-in `str`, an `Annotated` str or a simple
subclass of `str`. It also fixes/makes explicit the logic behind doing a
json parse of the value if it is untyped.

---------

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant