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

RunnerInput/Output feedback/issues #962

Closed
elliotgunton opened this issue Feb 15, 2024 · 2 comments · Fixed by #977
Closed

RunnerInput/Output feedback/issues #962

elliotgunton opened this issue Feb 15, 2024 · 2 comments · Fixed by #977
Assignees
Labels
type:bug A general bug

Comments

@elliotgunton
Copy link
Collaborator

elliotgunton commented Feb 15, 2024

Issue to collect feedback and bug reports for new experimental Runner IO

Bug reports

Bug 1: RunnerInput attributes require defaults

A RunnerInput like

class MyInput(RunnerInput):
    my_param: str

when building a yaml for a Workflow will give an error like:

...
  File "/Users/egunton/test-runner-input/.venv/lib/python3.11/site-packages/hera/workflows/io/v2.py", line 56, in _get_parameters
    parameters.append(Parameter(name=field, default=cls.model_fields[field].default))  # type: ignore
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/egunton/test-runner-input/.venv/lib/python3.11/site-packages/pydantic/v1/main.py", line 341, in __init__
    raise validation_error
pydantic.v1.error_wrappers.ValidationError: 1 validation error for Parameter
__root__
  Object of type PydanticUndefinedType is not JSON serializable (type=type_error)
make[1]: *** [build-yaml] Error 1

due to trying to create the default as default=cls.model_fields[field].default, when this is non-existent (PydanticUndefinedType).

Bug 2: Passing parameters for RunnerInput values errors out with JSONDecodeError

We should be calling parse, not json.loads here, which results in an error log like:

Traceback (most recent call last):
  File "/opt/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/opt/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/layers/...pip/requirements/lib/python3.9/site-packages/hera/workflows/runner.py", line 493, in <module>
    _run()
  File "/layers/...pip/requirements/lib/python3.9/site-packages/hera/workflows/runner.py", line 481, in _run
    result = _runner(args.entrypoint, kwargs_list)
  File "/layers/...pip/requirements/lib/python3.9/site-packages/hera/workflows/runner.py", line 423, in _runner
    kwargs = _map_argo_inputs_to_function(function, kwargs)
  File "/layers/...pip/requirements/lib/python3.9/site-packages/hera/workflows/runner.py", line 259, in _map_argo_inputs_to_function
    map_runner_input(param_name, func_param.annotation)
  File "/layers/...pip/requirements/lib/python3.9/site-packages/hera/workflows/runner.py", line 240, in map_runner_input
    matched_field = map_field(field)
  File "/layers/...pip/requirements/lib/python3.9/site-packages/hera/workflows/runner.py", line 235, in map_field
    mapped_kwargs[field] = json.loads(kwargs[field])
  File "/opt/lib/python3.9/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/opt/lib/python3.9/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/opt/lib/python3.9/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
time="2024-02-20T16:37:47 UTC" level=info msg="sub-process exited" argo=true error="<nil>"
Error: exit status 1

for the following Python code (same as #972):

class MyInput(RunnerInput):
    """A pydantic model."""

    my_param: str = ""


@script(constructor="runner")
def echo_my_param(my_input: MyInput) -> None:
    """Prints out a message."""
    print(my_input.my_param)
    values = [1]
    print(values[1])  # raise a keyerror


def get_workflow_template() -> WorkflowTemplate:
    """Create and return the test-runner-input WorkflowTemplate."""
    make_scripts_runnable()
    global_config.experimental_features["script_pydantic_io"] = True
    with WorkflowTemplate(
        name="test-runner-input",
        entrypoint="steps",
    ) as wt:
        with Steps(name="steps"):
            echo_my_param(arguments={"my_param": "Hello world"})
        return wt

Environment

  • Hera Version: 5.14.0
  • Python Version: 3.11.6
  • Argo Version: 3.4.16

Additional context
Add any other context about the problem here.

@elliotgunton elliotgunton added the type:bug A general bug label Feb 15, 2024
@elliotgunton
Copy link
Collaborator Author

Also see #961

@elliotgunton elliotgunton self-assigned this Feb 22, 2024
@elliotgunton elliotgunton mentioned this issue Feb 22, 2024
4 tasks
@elliotgunton
Copy link
Collaborator Author

On the Hera dev side, the new RunnerIO code in runner.py has made it pretty difficult to understand/navigate. Needs some tidying up and refactoring.

cc @samj1912 @flaviuvadan

elliotgunton added a commit that referenced this issue Mar 4, 2024
**Pull Request Checklist**
- [x] Fixes #962 
- [x] Tests added
- [ ] Documentation/examples added
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
Primarily, this PR refactors the runner code. It also fixes the mapping
and loading of kwargs to a RunnerInput object.

Currently, the runner code is hard to follow. This PR refactors the
functionality in the `runner.py` file into a `_runner` module with util
submodules. The PR also makes the complex input mapping logic more
testable.

---------

Signed-off-by: Elliot Gunton <[email protected]>
elliotgunton added a commit that referenced this issue Mar 4, 2024
**Pull Request Checklist**
- [x] Fixes #962 
- [x] Tests added
- [ ] Documentation/examples added
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
Primarily, this PR refactors the runner code. It also fixes the mapping
and loading of kwargs to a RunnerInput object.

Currently, the runner code is hard to follow. This PR refactors the
functionality in the `runner.py` file into a `_runner` module with util
submodules. The PR also makes the complex input mapping logic more
testable.

---------

Signed-off-by: Elliot Gunton <[email protected]>
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