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

Required Param requires None to be explicitly passed as default #28940

Closed
1 of 2 tasks
aamster opened this issue Jan 14, 2023 · 9 comments
Closed
1 of 2 tasks

Required Param requires None to be explicitly passed as default #28940

aamster opened this issue Jan 14, 2023 · 9 comments
Assignees
Labels
affected_version:2.5 Issues Reported for 2.5 area:core kind:bug This is a clearly a bug

Comments

@aamster
Copy link

aamster commented Jan 14, 2023

Apache Airflow version

2.5.0

What happened

I have a dag like

import datetime

from airflow.models import Param
from airflow.models.dag import dag


@dag(
    dag_id="foo",
    schedule=None,
    params={'month': Param()},
    start_date=datetime.datetime.now()
)
def foo():
    pass


foo()

I am trying to pass this param via CLI. I tried passing it like

airflow dags test foo --conf '{"month": "2022-01"}'

gives

Traceback (most recent call last):
  File "/Users/adam.amster/venvs/airflow/lib/python3.10/site-packages/airflow/models/dagbag.py", line 437, in _process_modules
    dag.validate()
  File "/Users/adam.amster/venvs/airflow/lib/python3.10/site-packages/airflow/models/dag.py", line 667, in validate
    self.params.validate()
  File "/Users/adam.amster/venvs/airflow/lib/python3.10/site-packages/airflow/models/param.py", line 230, in validate
    raise ParamValidationError(f"Invalid input for param {k}: {ve}") from None
airflow.exceptions.ParamValidationError: Invalid input for param month: No value passed and Param has no default value

Note

Found out that you need to explicitly pass None as the default, otherwise it will crash with above.

What you think should happen instead

month should be passed to the dag

How to reproduce

No response

Operating System

mac osx

Versions of Apache Airflow Providers

No response

Deployment

Virtualenv installation

Deployment details

No response

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@aamster aamster added area:core kind:bug This is a clearly a bug labels Jan 14, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jan 14, 2023

Thanks for opening your first issue here! Be sure to follow the issue template!

@aamster aamster changed the title Param does not work from command line Param requires None to be explicitly passed from command line if it is a required argument Jan 14, 2023
@aamster aamster changed the title Param requires None to be explicitly passed from command line if it is a required argument Param requires None to be explicitly passed as default Jan 14, 2023
@aamster aamster changed the title Param requires None to be explicitly passed as default Required Param requires None to be explicitly passed as default Jan 14, 2023
@potiuk
Copy link
Member

potiuk commented Jan 15, 2023

Marked it as good first issue. Hopefully somoene will fix it soon (but it's up for grabs for you if you would like to fix it ). PRs are most welcome to fix it.

@amoghrajesh
Copy link
Contributor

@potiuk @aamster I can take this up if it is not already taken. Would be more than happy to have this fixed.

@Taragolis
Copy link
Contributor

Found out that you need to explicitly pass None as the default, otherwise it will crash with above.

For me it make sense. If no default value provided than parameter is a mandatory, and otherwise if default parameter provided than parameter is optional

@hussein-awala
Copy link
Member

hussein-awala commented Feb 19, 2023

I agree with @Taragolis, we should check if the required parameters (with no default value) are provided or not, and fail the dag run if they are not provided.
I'm working on a #29174 to deprecate dag run conf and support providing the params directly, and this exception will be raised before creating the dag run (code)

@hussein-awala
Copy link
Member

It seems like currently we don't support required params, to fix this, I think we need to differentiate between parsing the params during the dag parsing/creation and parsing the params during the dag run execution.

During the dag parsing, we should not raise an exception if the default is not provided because this param is defined as required param. But during the dag run execution, if the param is required and the value is not provided, we should raise an exception to fail the dag run or the task if the params are defined at the task level.

I don't think this is a good first issue.

@potiuk
Copy link
Member

potiuk commented Feb 19, 2023

Sure. Removed the label then

@eladkal eladkal added the affected_version:2.5 Issues Reported for 2.5 label Feb 28, 2023
@aamster
Copy link
Author

aamster commented Jun 20, 2023

This is even worse for the case of a Param with type of i.e. int. Then you need to pass a default value like -999.

@jscheffl
Copy link
Contributor

There had been a couple of PRs by me, for example #31301 and #34248 which handle optional fields and how Params are handled for this. Just got note of this bug report and must state after a manual re-test: Can not be re-produced. Did not explicitly test but might have been resolved in 2.8.0 already. Therefore closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected_version:2.5 Issues Reported for 2.5 area:core kind:bug This is a clearly a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants