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

We should error on Output field usage in DAG/Steps #1222

Open
elliotgunton opened this issue Sep 30, 2024 · 4 comments · May be fixed by #1221
Open

We should error on Output field usage in DAG/Steps #1222

elliotgunton opened this issue Sep 30, 2024 · 4 comments · May be fixed by #1221
Assignees
Labels
type:bug A general bug

Comments

@elliotgunton
Copy link
Collaborator

Output is not mandated but you can e.g. set result or exit_code on it which should probably be an error within a DAG/Steps function.

Originally from #1162

@elliotgunton elliotgunton added the type:bug A general bug label Sep 30, 2024
@elliotgunton elliotgunton added this to the HEP0001 - new decorators milestone Sep 30, 2024
@elliotgunton elliotgunton self-assigned this Oct 1, 2024
@elliotgunton
Copy link
Collaborator Author

This may need something like https://github.com/pwwang/python-varname or https://github.com/alexmojaki/sorcery, or just a stack inspection mechanism to see if the function context is a dag/step-decorated function. We cannot simply check the _context is DAG or Steps, i.e.

        if "result" in kwargs or "exit_code" in kwargs and isinstance(_context, (DAG, Steps)):
            raise SyntaxError("Cannot use `result` or `exit_code` in dag or steps decorator functions.")

in

def __init__(self, /, **kwargs):
if _context.declaring:
# Return in order to skip validation of `construct`ed instance
return
super().__init__(**kwargs)

As there will be script function calls where Output(result="foo") is valid, but the _context is still DAG/Steps.

Slightly more complex, so will leave out of #1221 and come back to this. Also it's more of an enhancement than a bug fix

@elliotgunton elliotgunton added type:enhancement A general enhancement type:bug A general bug and removed type:bug A general bug type:enhancement A general enhancement labels Oct 1, 2024
@elliotgunton
Copy link
Collaborator Author

Actually, overcomplicating it for a basic fix - we can just check if result or exit_code are set in the object that the function return when constructing the dag, but we won't be able to raise an error on the line where the result or exit_code is set

elliotgunton added a commit that referenced this issue Oct 1, 2024
* 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 elliotgunton linked a pull request Oct 1, 2024 that will close this issue
3 tasks
@alicederyn
Copy link
Collaborator

We can have a separate validation flag in the context, and disable it temporarily in our script decorators?

@elliotgunton
Copy link
Collaborator Author

I don't think it's worth overcomplicating the codebase for now for a niche error case - the error text added in 5b56891 makes it obvious enough IMO

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.

2 participants