Prefer user signature over Hera-added overloads in @script typing

[alicederyn]

Pull Request Checklist

• Fixes #
• Tests added
• Documentation/examples added
• Good commit messages and/or PR title

Description of PR
Address review feedback in #1180 missed due to early merge.

• If we take a scenario where a user is adding the Hera library to their existing code and adds the @script decorator to a function, type checking should not be affected. We are an optional wrapper and should be treating the code as such.
• Preserve any docstring provided by the user for the overload which invokes the underlying code.
• Improve documentation for remaining overloads.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.6%. Comparing base (d553546) to head (547340b).
Report is 133 commits behind head on main.

Files with missing lines	Patch %	Lines
src/hera/workflows/script.py	66.6%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1181     +/-   ##
=======================================
- Coverage   81.7%   81.6%   -0.2%     
=======================================
  Files         54      60      +6     
  Lines       4208    4768    +560     
  Branches     889    1020    +131     
=======================================
+ Hits        3439    3891    +452     
- Misses       574     647     +73     
- Partials     195     230     +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[elliotgunton]

Minor doc string suggestions, but I would also update the name/argument types

[elliotgunton]

Could we update arguments in both overloads to Optional? We could also update name to be Optional as it technically is, within the magic syntax call.

[alicederyn]

• ArgumentsT is already Optional:

  hera/src/hera/workflows/_mixins.py

  Lines 133 to 138 in a50ba23

  	ArgumentsT = Optional[
  	Union[
  	ModelArguments,
  	OneOrMany[Union[Parameter, ModelParameter, Artifact, ModelArtifact, Dict[str, Any]]],
  	]
  	]

• I don't think name is Optional; if you pass in None, the default will be overwritten:

  hera/src/hera/workflows/script.py

  Lines 757 to 763 in a50ba23

  	if "name" in script_kwargs:
  	# take the client-provided `name` if it is submitted, pop the name for otherwise there will be two
  	# kwargs called `name`
  	name = script_kwargs.pop("name")
  	else:
  	# otherwise populate the `name` from the function name
  	name = source.__name__.replace("_", "-")

[elliotgunton]

ArgumentsT is already Optional

🤦Oops sorry! 😂

For the name, your linked code is for the @script part, i.e. users can do @script(name=None) which is allowed but would be a bit weird - it's allowed (name is optional) for the very niche use case of inline templates (they are kind of broken/unreliable and we tend to avoid using them though).

The Step/Task name comes from here:

hera/src/hera/workflows/_meta_mixins.py

Lines 331 to 332 in a50ba23

	if "name" not in kwargs:
	kwargs["name"] = self.name # type: ignore

But names for Steps/Tasks are mandatory, so my point is if you have a callable function doing my_function(arguments={...}), which is allowed (as we infer the name from the _meta_mixins.py lines above), wouldn't/shouldn't that raise a linting error?

For completeness, if you do my_function(name=None,...) you'd get:

examples/workflows/steps/callable_steps.py:14: in <module>
    hello(name=None, arguments={"name": "hello-1-{{inputs.parameters.my-step-input}}"})
src/hera/workflows/script.py:771: in task_wrapper
    return s.__call__(*args, **kwargs)
src/hera/workflows/_meta_mixins.py:378: in __call__
    return Step(template=self, **kwargs)

...

>           raise validation_error
E           pydantic.v1.error_wrappers.ValidationError: 1 validation error for Step
E           name
E             none is not an allowed value (type=type_error.none.not_allowed)

Which we should fix if we make it name: Optional = None

[alicederyn]

Whoops! Yep, your code snippet was what I was trying to link to. Let me restate my understanding to check we're on the same page. Right now, at runtime:

1. If name is provided and is a string, all is good.
2. If name is provided and None, there's a runtime error.
3. If name is not provided, it defaults to the name of the Script.

I expressed that as name: str = .... i.e. it must be a string but can be left at the default. Declaring it as Optional would only let the user pass in None, which doesn't match current behaviour.

We could change the implementation to allow None and then mark it as Optional in the overloads (I avoided any implementation changes), just for consistency. Is that the ask?

[alicederyn]

if you have a callable function doing my_function(arguments={...}), which is allowed (as we infer the name from the _meta_mixins.py lines above), wouldn't that raise a linting error?

Not with how I've written the type hints right now, no. test_optional_step_and_task_arguments validates this.

[alicederyn]

Just to further check we're on the same page, Optional doesn't let you leave off an argument, it lets you pass in None. = ... lets you leave off the argument (and has no effect on whether you can pass in None). The ... is a Protocol convention to avoid duplicating (or getting wrong) the default value.

[elliotgunton]

I actually didn't know about the ... protocol behaviour for defaults, but it seems like the best way to describe "there is a default, but it will be set at runtime (so we can't give an accurate type hint)." (Unless we can capture that? Not fussed right now though.)

I think the code is fine then, as I was just confused by the intellisense suggesting name is required because I didn't understand what the = ... type hint actually meant

[elliotgunton]

Good stuff, thank you! 🚀