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: Got 400 from Argo, a value was not supplied in the parameter #961

Closed
sam2x opened this issue Feb 14, 2024 · 2 comments · Fixed by #982
Closed

RunnerInput: Got 400 from Argo, a value was not supplied in the parameter #961

sam2x opened this issue Feb 14, 2024 · 2 comments · Fixed by #982

Comments

@sam2x
Copy link

sam2x commented Feb 14, 2024

Hello,

Thank you for this framework. I am facing an issue using "RunnerInput" with typical inputs i would like to structure.

Issue

The code is as below:

class DefaultParamsModel(BaseModel):
   name: str = "default value"
   my_var: str = "another default str"

class MyDefaultInput (RunnerInput):
    my_env: Annotated[DefaultParamsModel, Parameter(name="scope")] = DefaultParamsModel(
        name="test",
        my_var="bla",
    )
    stdout: Annotated[Path, Parameter(global_name="stdout", name="stdout", output=True)]
    stderr: Annotated[Path, Parameter(global_name="stderr", name="stderr", output=True)]

[...]

@script(constructor="runner") 
def ctn_test(my_input: MyDefaultInput) -> MyDefaultOutput: 
   [my_code...]
   return MyDefaultOutput(exit_code=exit_code, result="???", stdout=my_stdout, stderr=my_stderr,...)

I would like to use the statement below mentioned in the documentation:

Input-Output function parameters
Hera also allows output Parameter/Artifacts as part of the function signature when specified as a Path type, allowing users to write to the path as an output, without needing an explicit return. They require an additional field output=True to distinguish them from the input parameters and must have an underlying Path type (or another type that will write to disk).

I want to use annotation feature so I don't have to duplicate the declaration. In my context, the goal is to use a variable referenced in "inputs" (as parameter, so without having to create a "temporary" file), fill the content during execution, record it as artefact, and after the exit to be available to others tasks (as well as others output).

  1. How to use it with the RunnerInput class ?
  2. If i use an artifact declaration in the function signature, will it be recorded automatically in my registered hera s3 despite the fact that here I declare it as "Parameter" (throught the "stdout" variable, and it is not "written" by myself on the fs) ?
  3. Is it possible to inherit from an existing RunnerInput class to add extra field in a new class ? so I don't have to rewrite all the arguments, trying to do something like that (based on the sample above):
MySecondDefaultInput(MyDefaultInput): 
    new_var: Annotated[Path, Parameter(name="...", output=True)]
  1. It's still unclear for me the purpose of "result" with "RunnerOutput". In which use-case I would use it ?

Additional information

I am using latest version, I have embedeed the python lib to be run on the image. All works fine for this side. However for the declaration, I am not sure what i am doing wrong here.
Notes :

  • Expiremental Pydantic is enabled
  • Experimental Annotations is enabled
  • Latest version (5.14.0) is used

Thank you for any helps/pointers.

@elliotgunton
Copy link
Collaborator

Hi @sam2x - thank you for the feedback on this new feature! I will answer your questions numbered 1-4:

  1. Currently, the use of output=True is not explicitly supported by the RunnerInput class (and I haven't tested it). You would need to workaround Pydantic's validation by setting a default value. I think it might work this way, but no guarantees!
class MyDefaultInput (RunnerInput):
    stdout: Annotated[Path, Parameter(global_name="stdout", name="stdout", output=True)] = Path("/tmp/stdout.txt")
  1. I think the answer is "no" but I'm not exactly clear on what you're asking - "If i use an artifact declaration in the function signature" vs "here I declare it as "Parameter" - you can only declare it as one or the other? Hera does different things for you automatically based on the type of function parameter, so you would have to use S3Artifact to "be recorded automatically".
  2. Yes that should work - I can add a test to confirm it
  3. The result is meant to emulate the script result (see docs) - to allow steps/dags to use the syntax of {{steps.my-step.outputs.result}}

@sam2x
Copy link
Author

sam2x commented Feb 15, 2024

Hello @elliotgunton, thank you for the quick reply. I start to get more visibility on this.

  1. This was also my outcome, it doesnt seems yet supported under RunnerInput specific context usage.
  2. Just mixed my explanation. I thought this specific "output=True" would generate an "artifact" under the hood (since it provide a Path i would "write_text" to) without having to explicitly declare it in my "outputs" arguments.
  3. Awesome
  4. Oh, make sense now, thanks!

Following my exploration, It seems that the RunnerInput documentation sample is not working out-the-box. I used the documentation example:

class MyInput(RunnerInput):
    param_int: Annotated[int, Parameter(name="param-input")] = 42
    an_object: Annotated[MyObject, Parameter(name="obj-input")] = MyObject(
        a_dict={"my-key": "a-value"}, a_str="hello world!"
    )
    artifact_int: Annotated[int, Artifact(name="artifact-input", loader=ArtifactLoader.json)]

class MyOutput(RunnerOutput):
    param_int: Annotated[int, Parameter(name="param-output")] 
    artifact_int: Annotated[int, Artifact(name="artifact-output")]
    
@script(constructor="runner")
def pydantic_io(
    my_input: MyInput,
) -> MyOutput:
    return MyOutput(exit_code=1, result="Test!", param_int=42, artifact_int=my_input.param_int)

And in my workflow I tried several way to submit the pydantic_io :

  t_pydantic = pydantic_io()
Code Result Observation
pydantic_io() inputs.artifacts.artifact-input was not supplied` Legit, since there isn't default value and we haven't pass any value for artifact_int. But the doc doesnt mention anything about it.
pydantic_io(arguments=MyInput()) ValidationError: 1 validation error for MyInput artifact_int field required (type=value_error.missing) Similar call, so validation error make sense so far to me
pydantic_io(arguments=MyInput(artifact_int=0)) _Parameter.check_name (parameter.py:29) ValueError: name cannot be None or empty when used Expected, arguments is expected a dict of parameters/artifacts
pydantic_io(arguments={'my_input':MyInput(artifact_int=0)}) BadRequest: Server returned status code 400 with message: templates.main.tasks.pydantic-io templates.pydantic-io inputs.artifacts.artifact-input was not supplied I was hoping this one to work
args=MyInput(param_input=0, artifact_int=0) pydantic_io(arguments=args._get_parameters()) templates.main.tasks.pydantic-io.arguments.param-input.value is required Lost on this one, param-input is supposed to have default value already set, but i guess i am calling it wrongly
  1. Is there a working way to pass the missing value artifact_int ?
  2. What is the conventional way to "overwrite" a specific parameter (such as "my_object") ?

Thank you

@elliotgunton elliotgunton linked a pull request Feb 26, 2024 that will close this issue
4 tasks
elliotgunton added a commit that referenced this issue Mar 4, 2024
* Script user guide was too long, split into main features. Fix internal
links
* Make pydantic io example into a runnable workflow - made it more
obvious the scripts would need a custom image

**Pull Request Checklist**
- [x] Fixes #961 (fixes example)
- [ ] Tests added
- [x] Documentation/examples added
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
Currently, the pydantic IO example does not work. Combined with fixes in
PRs #974 and #977, this doc change shows how to use the Runner IO
features.

---------

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
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants