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

Parameters not rendered correctly in 2.7.0 UI #33923

Closed
1 of 2 tasks
matteoannotell opened this issue Aug 30, 2023 · 12 comments
Closed
1 of 2 tasks

Parameters not rendered correctly in 2.7.0 UI #33923

matteoannotell opened this issue Aug 30, 2023 · 12 comments
Assignees
Labels
affected_version:2.7 Issues Reported for 2.7 area:UI Related to UI/UX. For Frontend Developers. kind:bug This is a clearly a bug
Milestone

Comments

@matteoannotell
Copy link

Apache Airflow version

2.7.0

What happened

When initialising integer parameters as 0, or arrays as empty list, the default behaviour of the UI is to render it as null.
This results in default parameters being ignored unless re-imputed manually.
Passing a config like this:

params={
        "integer_param": Param(0, type="integer", minimum=0),
        "list_param": Param([], type="array"),
    },

results in this in the UI when the DAG loads, forcing the user to manually input again 0, or editing the json for having an empty list
image

image

What you think should happen instead

0 rendered as 0, empty lists as empty lists

How to reproduce

This DAG should be an MVP for reproducing the error

from datetime import datetime

from airflow import DAG
from airflow.models.param import Param
from airflow.operators.python_operator import PythonOperator

default_args = {
    "owner": "data-enablement",
    "start_date": datetime(2023, 4, 24),
    "retries": 30,
    "startup_timeout_seconds": 600,
}

with DAG(
    "test-error",
    default_args=default_args,
    schedule=None,
    params={
        "integer_param": Param(0, type="integer", minimum=0),
        "list_param": Param([], type="array"),
    },
    render_template_as_native_obj=True,
    description="MVP for reproducing error",
) as dag:
    print_int_task = (
        PythonOperator(
            task_id="print_int",
            op_args=[
                "{{ params.integer_param}}",
            ],
            python_callable=(lambda integer_param: print(integer_param)),
        ),
    )

    print_list_task = PythonOperator(
        task_id="print_list",
        op_args=[
            "{{ params.list_param}}",
        ],
        python_callable=(lambda list_param: print(list_param)),
    )

print_int_task, print_list_task

Operating System

macOS M1 locally

Versions of Apache Airflow Providers

No response

Deployment

Docker-Compose

Deployment details

Using this image:
https://hub.docker.com/layers/apache/airflow/2.7.0-python3.10/images/sha256-d6cb37572c21546ba3e60274d3cc1e782cc517a31443b8051702b19584e7379b?context=explore

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@matteoannotell matteoannotell added area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Aug 30, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 30, 2023

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@potiuk potiuk added this to the Airflow 2.7.1 milestone Aug 30, 2023
@RNHTTR RNHTTR added area:UI Related to UI/UX. For Frontend Developers. affected_version:2.7 Issues Reported for 2.7 and removed area:core needs-triage label for new issues that we didn't triage yet labels Aug 30, 2023
@SamWheating
Copy link
Contributor

I can look into this, feel free to assign to me.

@josh-fell
Copy link
Contributor

All yours @SamWheating!

@jscheffl
Copy link
Contributor

jscheffl commented Aug 31, 2023

Empty lists being rendered as null (also the same for empty strings and objects) was actually a desire. This is the (only?) way to enforce users to enter values into lists for the required input check. If we would change the nullthing we need to think about required filed validation as well and make it consistent across all types.
If you want to have lists optional then you need to use type=["null", "array"].

I agree the integer thing is actually a bug. With 0 as default value a 0 should be accepted.

I'd also be OK to contribute a fix if a desire is having this on short term.

Update: I also just realized the "null"/None field behavior is documented in https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/params.html

"If a form field is left empty, it is passed as None value to the params dict."

@matteoannotell
Copy link
Author

Empty lists being rendered as null (also the same for empty strings and objects) was actually a desire.

But then if one defaults the value of a parameters to an empty list, and build everything that uses it around the premises of receiving an iterable, things will break when receiving None, which is not iterable?

@jscheffl
Copy link
Contributor

Question is what "everything" means in your case. In Jinja you can {{ params.mylist | default([]) }} and inside Python code it would be for item in params.mylist or []: ... if you require to loop w/o checking the content. Main purpose of typing validation is that you can trust submitted params before handling it in the code. If you field is not required you anyway need to take care to handle "missing data" for example if called via CLI or REST API.

On the other hand if an empty list would be passed as [] to the DAG, validation for required fields would be harder. Then you need to add a minItems schema definition if you want to enforece that users add at least one item.. Makes the form validation much more complex, standard jQuery mechanisms for required fields are not working easily in this example.

I'd prefer to keep it consistent that "missing data" is passed as null/None as it is stated in the docs currently. Otherwise we would need to define the empty handling different per type and document this.

@matteoannotell
Copy link
Author

I see your point, but for item in params.mylist or [] feels like a double control if the param is already specified to be an array in Param([], type="array")?
Besides that, there is also the minor annoyance of having to manually providing a parameter to the UI, otherwise the DAG cannot be triggered.
Neither of my points are true dealbreakers and I agree that there are simple workarounds, but it feels like something that should/could (?) perhaps work out of the box

image

@SamWheating
Copy link
Contributor

I agree that there should be a way to pass either a null or an empty list through the form interface without any workarounds, but I think it will require some more changes (maybe we'll have to include the [ ] in the form field?).

I'll draft up some possible implementations and we can chat on the PR.

@jscheffl
Copy link
Contributor

If the field is a list and is required anyway then I would expect that also at least one element needs to be returned - an empty list for me does not represent a valid value if defined as Param([], type="array"). Or is your use case expecting that the user can pass no element in the array as valid option?

The main difference we have is how to define a "non required or empty array" as result and technically all is possible. But the form can not distinguish whether [] or None is expected.
We have the same matter also with all other non-bool types (string, int, object), an empty string field also is passed as None. I still opt for consistency.

@SamWheating
Copy link
Contributor

We have the same matter also with all other non-bool types (string, int, object), an empty string field also is passed as None. I still opt for consistency.

I think that for any of these there is a clear difference between "empty" values ("", 0, {}, []) and null values, and we should probably allow users to provide either the empty or null value as required. So while I agree that we should have consistency, I think I disagree on what that consistent interface should look like.

I also acknowledge that this definitely adds some complexity implementation-side and probably won't make any difference for 99% of use cases.

Or is your use case expecting that the user can pass no element in the array as valid option?

Yes, I think that both null and [] are valid but distinctly different.

@matteoannotell
Copy link
Author

Or is your use case expecting that the user can pass no element in the array as valid option?

Yes, empty list would be a valid option in my DAG (with all discussed so far still holding, I could just rewrite the implementation so that null is allowed and the rest of the DAG is adjusted accordingly)

@jscheffl
Copy link
Contributor

jscheffl commented Sep 1, 2023

I'd propose to close this ticket and separate-out the empty list thing into a second discussion/feature ticket. This will not make it (including discussion) into 2.7.1.
The main bug of missing rendering a 0 number value was fixed in #33965

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected_version:2.7 Issues Reported for 2.7 area:UI Related to UI/UX. For Frontend Developers. kind:bug This is a clearly a bug
Projects
None yet
Development

No branches or pull requests

6 participants