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

UI: New trigger DAG with config form doesn’t allow valid floats and empty strings #31930

Closed
1 of 2 tasks
mikelius2b opened this issue Jun 15, 2023 · 5 comments
Closed
1 of 2 tasks
Assignees
Labels
area:core area:UI Related to UI/UX. For Frontend Developers. kind:bug This is a clearly a bug Stale Bug Report

Comments

@mikelius2b
Copy link

Apache Airflow version

2.6.1

What happened

There are two problems with new form which forms DAG config.

1. It doesn't allow to fill valid float with two or two decimal places

This param in DAG code looks like: Param(default=None, type=["number", "null"]
But in form we get:
Screenshot 2023-06-15 at 13 23 28

2. It doesn't considers empty string as filled parameter

  • in DAG we have param 'param1': Param(default='', type='string')
  • expectation is that we don't need to fill such param in form
  • but form asks us to fill it
    Screenshot 2023-06-15 at 13 32 48

3. There is some strange behavior in interaction between filled form and "Generated Configuration JSON"

  • we fill form
  • we paste manually created JSON into configuration field
  • DAG runs with configuration from JSON

This helped us to overcome problem from p.1 and p.2, but it seems not very reliable, probably parameters in form should be synced with JSON.

What you think should happen instead

Problem 1: field allows to fill proper float values.
Problem 2: field don't need to be filled
Problem 3: values in JSON synced with values in form

How to reproduce

Problem 1:

  1. Create DAG with number param
  2. Try to fill 0.01 into this param field in Trigger DAG with config form

Problem 2:

  1. Create DAG with param Param(default='', type="string"
  2. Try run DAG with config without filling corresponding field

Problem 3:

  1. Create DAG with some params
  2. Go to Trigger DAG page and fill parameters in form
  3. Insert custom JSON in "Generated Configuration JSON"
  4. Run DAG

Operating System

MacOS

Versions of Apache Airflow Providers

No response

Deployment

Official Apache Airflow Helm Chart

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

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

boring-cyborg bot commented Jun 15, 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.

@hussein-awala hussein-awala self-assigned this Jun 15, 2023
@jscheffl jscheffl added area:UI Related to UI/UX. For Frontend Developers. and removed needs-triage label for new issues that we didn't triage yet labels Jun 16, 2023
@jscheffl
Copy link
Contributor

Thanks for reporting this bug - and as Part 1 is already fixed with a PR let me answer on the others.

Regarding Part 2) Empty strings are converted to null values as parameters and if you define a field to be string it is by JSON Schema validation a required field. If you want to have it optional you need to define it as 'param1': Param(default='', type=['null', 'string'])

Regarding Part 3) The implementation of the user form was made in mind that the form fields generate the actual JSON dict which is used to trigger the DAG. It was included in the form because it was just a dict before and for the case you want to manually inspect or tune what is being submitted (or like in your case if you find a problem that you are not blocked). For power user it is a way that you can copy&paste a previous JSON you might have captured from the past. It was not intended to be a 2-way sync back if you modify the JSON back to form fields. This might be a bit more complex to be implemented. I feared (when I contributed the code) this might be a bit over-kill and additional complexity generates more bugs. But if you like to contribute and feel this would be a good feature please feel free to open a PR to contribute.

Therefore I assume the bug is resolved, if you have a different opinion, please re-open.

@hussein-awala
Copy link
Member

@jens-scheffler-bosch Actually I think we can improve the second part, for that I didn't closed the issue after fixing the first part.
I will re-open it and check if the fix/feature can be implemented easily without changing any behavior.

Copy link

This issue has been automatically marked as stale because it has been open for 365 days without any activity. There has been several Airflow releases since last activity on this issue. Kindly asking to recheck the report against latest Airflow version and let us know if the issue is reproducible. The issue will be closed in next 30 days if no further activity occurs from the issue author.

Copy link

This issue has been closed because it has not received response from the issue author.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core area:UI Related to UI/UX. For Frontend Developers. kind:bug This is a clearly a bug Stale Bug Report
Projects
None yet
Development

No branches or pull requests

3 participants