jens-scheffler-bosch opened a new pull request, #27063: URL: https://github.com/apache/airflow/pull/27063
This PR is a (draft/PoC) implementation for a trigger UI based on ideas and discussion from [AIP-50 Trigger DAG UI Extension with Flexible User Form Concept](https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-50+Trigger+DAG+UI+Extension+with+Flexible+User+Form+Concept) based on current FAB UI infrastructure. Due to the feedback received on [AIP-50](https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-50+Trigger+DAG+UI+Extension+with+Flexible+User+Form+Concept) the PR is based on the [`Params` toolings](https://airflow.apache.org/docs/apache-airflow/stable/concepts/params.html) already available and leverages with minimal changes 80% of desired scope. Coverage/Scope of this PR: - AIP-50 Part 1+3 (fully covered): Specifying Trigger UI on DAG Level + Standard Implementation for Forms - By defining `params` or usage of existing `params` dict a form as UI is generated dynamically - Existing JSON schema is used as good as possible with slight extensions for purpose of form rendering - Compared to the initial idea to have a "new" form as dialog, the proposed PR now extends the existing JSON entry UI by: - If `params` are defined on a DAG a form is generated - If no `params` are defined the previous JSON based entry is presented - Why? Benefits? Part 2 was not implemented and showing a form if no parameters are defined is not good (=empty form) and the current proposal also allows modification of JSON if needed. Two dialogs might also confuse users (?) where in this proposal one provides all functionality needed. Also the extension here in this PR is very thin, therefore no additional UI is needed in our opinion. - Small derivations compared to current AIP-50: - Field type and rendering of field is not only made depending on Schema but also auto-discovered for common data types (number, list, json, boolean) which adds an easy entry for existing DAGs w/o needs of change == for free. - Field type `hidden` was implemented by using the JSON schema attribute `constant` which assumes that only one constant value is permitted, no additional flag needed. - Advanced section of a entry form was generalized allowing to define any/multiple sections of the form (not limited to one "Advanced") - AIP-50 Part 2 (NOT covered): UI Changes for Trigger Button - During implementation we saw that either performance would drain or DB schema would need to be extended besides anticipated code changes - As assumption is that part 2 can be separated and have a follow-up discussion for solution space we decided to take this out for the preview - As also no additional form was implemented, so no changes in the UI for triggering needed for the moment. - AIP-50 Part 4 (fully covered): Examples: - Two examples added within this PR - One simple example as "showcase" - One "Tutorial" with a lot of examples for large coverage and template for users to be able to copy - AIP-50 Part 5 (NOT covered): Documentation - Skipped this in the WIP state, if review is positive of course will be added. Until then the tutorial example DAG is the best way to understand how it works. - Proposal is to extend the existing page [Params](https://airflow.apache.org/docs/apache-airflow/stable/concepts/params.html) to add details about UI, options and supported parameters etc. Note that I skipped adding unit tests, these will be added (together with documentation) before merge if positive feedback is showing a way forward. Please review and provide feedback :-D Questions that I am explicitly interested in during the review: - What would be additional merge pre-requisites to move ahead? - (I am an positive thinker) Could this PR already be added early into a next 2.4.x release or will it need to get into a 2.5.0 release? Any condition to bring this to the field early? - Is it accepted to add this (small==680 LOC) change to FAB ui even knowing that this will be replaced by AIP-38 in "soon" time? Structure-wise I assume the implementation except the UI can be taken over w/o a breaking change. - Discussion about AIP-50 Part 2) The gap which has been discovered during the implementation of this PR is that the DAG overview page is rendered by a DB query. To bring the part 2) into this we we either (1) need to extend the DB scheme so that the property of the run options can be fetched from DB quick, or (2) we would need to load the `DagBag` of each row to discover options for UI trigger (which would be N=25 additional queries to DB) which slows down responsiveness or (3) we load the details of the Run buttons async which adds complexity for a convenience option - or (4) we just leave it as it is. (4) also means that this PR would complete the AIP-50. - I have noticed that if somebody use the trigger Dag Run button without parameters form then always an empty Dict is passed as `DagRun.conf`, which means a DAG must always be prepared to accept parameters but also might be called without. Also this conflicts to the idea that parameters are schema validated if they can still be totally missing. Would it make sense to (1) change the default behavior so that the default trigger (w/o form or scheduled) is changed such that the parameters are supplied always per default? Would ease DAG logic not needing to handle all situations when parameters are missing. Otherwise (2) it might be a good configuration option per DAG to define if parameters shall be taken as `DagRun.conf` if none are given. See also discussion in [Airflow Slack](https://apache-airflow.slack.com/archives/CCQ7EGB1P/p1665651809736739). Open Items before any merge/leaving WIP state: [ ] Positive response from reviewer(s) [ ] Pytests added [ ] Documentation added related: #26215 closes: #11054 FYI @clellmann @wolfdn -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
