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

Support Literal types in script runner #1173

Closed
2 of 6 tasks
alicederyn opened this issue Aug 23, 2024 · 0 comments · Fixed by #1249
Closed
2 of 6 tasks

Support Literal types in script runner #1173

alicederyn opened this issue Aug 23, 2024 · 0 comments · Fixed by #1249
Assignees
Labels
type:bug A general bug

Comments

@alicederyn
Copy link
Collaborator

Pre-bug-report checklist

1. This bug can be reproduced using pure Argo YAML

If yes, it is more likely to be an Argo bug unrelated to Hera. Please double check before submitting an issue to Hera.

2. This bug occurs in Hera when...

  • exporting to YAML
  • submitting to Argo
  • running on Argo with the Hera runner
  • other:

Bug report

Describe the bug
A clear and concise description of what the bug is:

Using a function with a Literal argument in the runner results in the following trace:

Error log if applicable:

Traceback (most recent call last):
  File "/.../python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File ".../python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File ".../python3.9/site-packages/hera/workflows/runner.py", line 6, in <module>
    _run()
  File "/.../python3.9/site-packages/hera/workflows/_runner/util.py", line 254, in _run
    result = _runner(args.entrypoint, kwargs_list)
  File ".../python3.9/site-packages/hera/workflows/_runner/util.py", line 228, in _runner
    return function(**kwargs)
  File ".../python3.9/site-packages/hera/workflows/_runner/util.py", line 58, in inner
    filtered_kwargs = {key: _parse(value, key, f) for key, value in kwargs.items() if _is_kwarg_of(key, f)}
  File ".../python3.9/site-packages/hera/workflows/_runner/util.py", line 58, in <dictcomp>
    filtered_kwargs = {key: _parse(value, key, f) for key, value in kwargs.items() if _is_kwarg_of(key, f)}
  File ".../python3.9/site-packages/hera/workflows/_runner/util.py", line 89, in _parse
    if _is_str_kwarg_of(key, f) or _is_artifact_loaded(key, f) or _is_output_kwarg(key, f):
  File ".../python3.9/site-packages/hera/workflows/_runner/util.py", line 142, in _is_str_kwarg_of
    return issubclass(type_, str)
TypeError: issubclass() arg 1 must be a class

To Reproduce
Full Hera code to reproduce the bug:

from typing import Literal
from hera.workflows import Workflow, script

@script(constructor="runner", image="my-image")
def literal_str(my_str: Literal["foo", "bar"]):
    print(my_str)

with Workflow(name="my-workflow", entrypoint="steps") as w:
    with Steps(name="steps"):
        literal_str(arguments={"my_str": "foo"})

Expected behavior
A clear and concise description of what you expected to happen:

  • For unsupported types, the error should indicate which argument had the issue, and that it is due to an unsupported type; at the moment, the key is lost, and the error message is not obvious.
  • Ideally:
    • no error
    • _is_str_kwarg_of should return True
    • Parameter.enum should be inferred to be ["foo", "bar"]

Environment

  • Hera Version: 5.16.2
  • Python Version: 3.9.18
  • Argo Version: 3.4.16.7
@alicederyn alicederyn added the type:bug A general bug label Aug 23, 2024
@alicederyn alicederyn self-assigned this Oct 10, 2024
sambhav pushed a commit that referenced this issue Oct 21, 2024
**Pull Request Checklist**
- [ ] Fixes #<!--issue number goes here-->
- [X] Tests added
- [ ] Documentation/examples added
- [X] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
PR #1168 changed the logic of `map_runner_input` and `_parse` to no
longer pass incoming values to `json.loads` if their annotated type was
a union that included str, rather than only when given a subtype of
`Optional[str]`. This makes it impossible to, for instance, pass an int
to a `Union[str, int]`.

This PR splits `origin_type_issubclass` into two functions,
`origin_type_issupertype` (which matches the previous behaviour) and
`origin_type_issubtype`, and use the latter instead to restore the
original behaviour.

Additionally, it fixes a bug where we were passing an annotation to
`issubclass` without first checking if it's a type, resulting in a
`TypeError` (partially addresses #1173).

---------

Signed-off-by: Alice Purcell <alicederyn@gmail.com>
sambhav pushed a commit that referenced this issue Nov 6, 2024
**Pull Request Checklist**
- [X] Fixes #1173
- [X] Tests added
- [ ] Documentation/examples added
- [X] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
Currently, the script runner does not support `Literal` types, as
`origin_type_issubtype` does not special-case them, so
`_is_str_kwarg_of` does not return True.

This PR adds supports for Literals to `origin_type_issubtype`, and
additionally copies their values from the annotation into the `enum`
field on input Parameters. It also combines two paths in
`_get_inputs_from_callable` into one by using
`construct_io_from_annotation` instead of having a fallback branch if
`get_workflow_annotation` returns `None`; this fixes a bug where the
default was being verified as None for optional strings only if there
was no IO annotation.

---------

Signed-off-by: Alice Purcell <alicederyn@gmail.com>
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