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.

Sign up for GitHub

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

Jump to bottom

Fix various bugs in dag/steps decorator #1221

Merged
merged 17 commits into from
Oct 3, 2024
Merged

Fix various bugs in dag/steps decorator #1221

merged 17 commits into from
Oct 3, 2024

Conversation

elliotgunton
Copy link
Collaborator

@elliotgunton elliotgunton commented Sep 30, 2024 •
edited
Loading

Pull Request Checklist

Description of PR
Various syntax-focused fixes from the issue, including (by commit order)

  • Function inputs for new-decorator functions can have no inputs, or one input which must be a subclass of the special Input class, anything else now raises an error
  • DAG Task/Step names no longer use underscores
  • Passing Input in the kwargs to a template call within a DAG function will now error (instead of silently ignoring)
  • Block use of new script decorator without special Input class
  • Add ArtifactLoaders to example where needed
  • Don't add a path for Steps/DAG artifact inputs (which would be a lint error)
  • Ignore Step/Task kwargs to allow local running
  • Keep Parameter/Artifact details for Steps/DAG outputs when hoisting output
  • Fix incorrect returns for Steps/DAG
  • Do not allow setting result or exit_code for Steps/DAG
  • Fix adding Steps/DAGs to TemplateSets

@elliotgunton
Allow no arg for dag decorator function
d7dbe1b 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Replace underscores in step/dag tasks
aafa3d5 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Raise error if Input in kwargs
ccb7d4b 
* Add `clear_context` fixture as global context was causing failures in
tests unrelated to new decorators (kwarg error test will exit the context
in an error, leaving it with `declaring == True`)

Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Add error for new script decorator incorrect use
24d6ff6 
* Function input must be none or a single arg (of Input type subclass)

Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Add ArtifactLoaders to new_dag_decorator_artifacts
7a79a66 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton elliotgunton added semver:patch A change requiring a patch version bump type:bug A general bug labels Sep 30, 2024
@elliotgunton
Don't add path for Steps/DAG artifact inputs
19be598 
* Refactors _get_artifacts and _get_inputs to with add_missing_path input var,
with default = False to match the _get_outputs equivalent

Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton elliotgunton marked this pull request as ready for review September 30, 2024 15:26
@elliotgunton
Ignore func kwargs when running locally
df6819b 
* `kwargs` will only contain Task/Step kwargs so if the function is being
called outside of a Hera context or during the decorator setup, we can
just drop the kwargs and call func(*args) (which will only contain a Hera Input)

Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton elliotgunton mentioned this pull request Oct 1, 2024
Closed
alicederyn
alicederyn suggested changes Oct 1, 2024
View reviewed changes
src/hera/workflows/_meta_mixins.py Outdated Show resolved Hide resolved
src/hera/workflows/_meta_mixins.py Outdated Show resolved Hide resolved
src/hera/workflows/_meta_mixins.py Outdated Show resolved Hide resolved
tests/test_unit/test_io_mixins.py Show resolved Hide resolved
@elliotgunton
Fix output hoisting losing values for DAG/Steps
3f53b63 
* Fixes #1218
* Parameter/Artifact fields (apart from the name) were not being carried
through, we can use the annotation and change value_from/from_ directly
* We can now using Annotated and leave out the name from Parameters/Artifacts,
so we need the name from the field when hoisting output

Signed-off-by: Elliot Gunton <[email protected]>
@alicederyn

This comment was marked as outdated.

Sign in to view
@elliotgunton
fixup! Allow no arg for dag decorator function
a2cf05b 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
fixup! Replace underscores in step/dag tasks
250a274 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
fixup! Fix output hoisting losing values for DAG/Steps
372c29a 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
fixup! Don't add path for Steps/DAG artifact inputs
615045e 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Fix bad returns from dag/steps
06bc5ae 
* Fixes #1219
* Inform user they are doing funky stuff with an error

Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Raise error if result or exit_code is set in dag
5b56891 
* Fixes #1222
* Setting result or exit_code for a dag/steps function doesn't make sense,
check if they are non-default values on return from the dag setup function
and error if so.

Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Allow template set as a context in add_sub_node
20fd421 
* Fixes #1170
* Add a DAG to the template set example
Signed-off-by: Elliot Gunton <[email protected]>
alicederyn
alicederyn reviewed Oct 2, 2024
View reviewed changes
src/hera/workflows/_meta_mixins.py Outdated Show resolved Hide resolved
src/hera/workflows/_meta_mixins.py Outdated Show resolved Hide resolved
@alicederyn

This comment was marked as resolved.

Sign in to view
@elliotgunton
Copy link
Collaborator Author

update the user guide now kwargs are not supported

That example was wrong to begin with 😅

@alicederyn
Copy link
Collaborator

alicederyn commented Oct 2, 2024 •
edited
Loading

Should we write code to test the docstrings in the user guide?

@elliotgunton
Copy link
Collaborator Author

Should we write code to test the docstrings in the user guide?

I don't think it's worth doing this for now, if we just copy/paste from an example workflow in the examples folder we should be good. It would be better to add linting #1163 for all the examples

@elliotgunton
Fix name=None case, improve dag return code error checking
1d80e4b 
* Allow users to pass in `name=None` in their Tasks (e.g. if they have
parameterised Task creation)
* Check that the object returned from the dag has a class matching the one
declared in the function signature, but cannot be a subclass

Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton
Update user guide using wrong syntax
9cd73bb 
Signed-off-by: Elliot Gunton <[email protected]>
@elliotgunton elliotgunton merged commit 1b0cd7e into main Oct 3, 2024
20 checks passed
@elliotgunton elliotgunton deleted the fix-dag-decorator branch October 3, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sambhav sambhav sambhav approved these changes

@alicederyn alicederyn alicederyn approved these changes

@flaviuvadan flaviuvadan Awaiting requested review from flaviuvadan flaviuvadan is a code owner

Assignees
No one assigned
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Milestone
No milestone
3 participants
@elliotgunton @alicederyn @sambhav