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

Add support to filter by last dagrun state in UI. #42779

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

tirkarthi
Copy link
Contributor

closes: #42715
related: #42715

Add filter state on change to "lastrun" URL parameter which is used in legacy UI too and pass the state to dags list API.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Oct 6, 2024
@tirkarthi
Copy link
Contributor Author

I am trying to run pnpm lint:fix which seems to give a different output compared to the errors in CI. Is there something missing in my local setup?

$ /home/karthikeyan/.local/share/pnpm/pnpm lint:fix

> [email protected] lint:fix /home/karthikeyan/stuff/python/airflow/airflow/ui
> eslint --fix && tsc --p tsconfig.app.json


/home/karthikeyan/stuff/python/airflow/airflow/ui/src/App.test.tsx
  35:20  warning  Use `undefined` instead of `null`  unicorn/no-null
  43:21  warning  Use `undefined` instead of `null`  unicorn/no-null
  45:21  warning  Use `undefined` instead of `null`  unicorn/no-null
  54:18  warning  Use `undefined` instead of `null`  unicorn/no-null
  55:23  warning  Use `undefined` instead of `null`  unicorn/no-null
  64:20  warning  Use `undefined` instead of `null`  unicorn/no-null
  72:21  warning  Use `undefined` instead of `null`  unicorn/no-null
  74:21  warning  Use `undefined` instead of `null`  unicorn/no-null
  83:18  warning  Use `undefined` instead of `null`  unicorn/no-null
  84:23  warning  Use `undefined` instead of `null`  unicorn/no-null

/home/karthikeyan/stuff/python/airflow/airflow/ui/src/pages/DagsList/DagsFilters.tsx
  30:28  warning  Arrow function has too many statements (12). Maximum allowed is 10  max-statements

/home/karthikeyan/stuff/python/airflow/airflow/ui/src/utils/index.tsx
  20:1   error  Expected a function expression                                                                             func-style
  27:10  error  Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly          @typescript-eslint/strict-boolean-expressions
  27:39  error  Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator  @typescript-eslint/prefer-nullish-coalescing

✖ 14 problems (3 errors, 11 warnings)

 ELIFECYCLE  Command failed with exit code 1.

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Oct 7, 2024

@tirkarthi

On my end, running pnpm lint:fix on your branch shows the same errors that the CI mentions.

It looks like your eslint is showing errors, where as the CI has no error on eslint part and shows errors from typescript only.

Can you verify your pnpm, eslint and node versions? You can find more information about supported versions in https://github.com/apache/airflow/blob/main/contributing-docs/14_node_environment_setup.rst

@bbovenzi
Copy link
Contributor

bbovenzi commented Oct 7, 2024

Yes, sometimes I need to make sure my local environment has the right version of node by running
nvm install 20 && nvm use 20

@tirkarthi
Copy link
Contributor Author

tirkarthi commented Oct 8, 2024

Thanks @pierrejeambrun and @bbovenzi . I had an uncommitted file with eslint error at src/utils/index.tsx. Removing it helped in fixing the issue.

TypeScript was complaining about typecasting the query parameter which was a string to type DagRunState. I did a typecast using "as". Googling around recommends using const with array of string but would be helpful to know if there is a similar pattern check in the existing code for reuse. The issue would be around passing invalid lastrun value that will be passed to API and 422 will be returned resulting in empty page.

type DagRunState = "queued" | "running" | "success" | "failed";

https://stackoverflow.com/questions/36836011/checking-validity-of-string-literal-union-type-at-runtime

@bbovenzi
Copy link
Contributor

bbovenzi commented Oct 8, 2024

https://stackoverflow.com/questions/36836011/checking-validity-of-string-literal-union-type-at-runtime

We don't have an example of that yet. But I imagine we'll need a util like that. In the stackoverflow link. I would definitely lean towards a version of option 3.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, working as expected.

A few minor suggestions :)

airflow/ui/src/pages/DagsList/DagsFilters.tsx Outdated Show resolved Hide resolved
airflow/ui/src/pages/DagsList/DagsFilters.tsx Outdated Show resolved Hide resolved
airflow/ui/src/pages/DagsList/DagsFilters.tsx Outdated Show resolved Hide resolved
airflow/ui/src/pages/DagsList/DagsFilters.tsx Show resolved Hide resolved
airflow/ui/src/pages/DagsList/DagsFilters.tsx Show resolved Hide resolved
@tirkarthi
Copy link
Contributor Author

https://stackoverflow.com/questions/36836011/checking-validity-of-string-literal-union-type-at-runtime

We don't have an example of that yet. But I imagine we'll need a util like that. In the stackoverflow link. I would definitely lean towards a version of option 3.

@bbovenzi Quoting from the answer this needs the dag run states to be an array of strings. Since it's a type using union syntax I guess it's tricky. Please add in if I missed something or to change something since the part seems to be auto generated.

In all cases you will need to store the strings in an object that is available at runtime (e.g. an array).

@tirkarthi
Copy link
Contributor Author

Thanks @pierrejeambrun and @bbovenzi , made the suggested changes and rebased with latest main branch.

Slightly off-topic but I see PRs with only new UI folder changes running tests for databases like mysql, postgres and sqlite which takes 20 minutes each along with integration tests, docs builds (--docs-only and spellcheck take 20+ mins) which I also guess is not required. Skipping them can speed up UI PRs build time and also reduce CI load. I tried to figure out how to skip them which should probably be done in selective_check function in ci_commands.py but couldn't figure out the exact conditional and logic.

https://github.com/apache/airflow/actions/runs/11229469726/job/31215226666 (20 minutes per database)
https://github.com/apache/airflow/actions/runs/11229469726/job/31215228586 (integration tests)

@bbovenzi
Copy link
Contributor

bbovenzi commented Oct 8, 2024

https://stackoverflow.com/questions/36836011/checking-validity-of-string-literal-union-type-at-runtime

We don't have an example of that yet. But I imagine we'll need a util like that. In the stackoverflow link. I would definitely lean towards a version of option 3.

@bbovenzi Quoting from the answer this needs the dag run states to be an array of strings. Since it's a type using union syntax I guess it's tricky. Please add in if I missed something or to change something since the part seems to be auto generated.

In all cases you will need to store the strings in an object that is available at runtime (e.g. an array).

Perhaps something like this: https://stackoverflow.com/a/70694878

@tirkarthi
Copy link
Contributor Author

https://stackoverflow.com/questions/36836011/checking-validity-of-string-literal-union-type-at-runtime

We don't have an example of that yet. But I imagine we'll need a util like that. In the stackoverflow link. I would definitely lean towards a version of option 3.

@bbovenzi Quoting from the answer this needs the dag run states to be an array of strings. Since it's a type using union syntax I guess it's tricky. Please add in if I missed something or to change something since the part seems to be auto generated.

In all cases you will need to store the strings in an object that is available at runtime (e.g. an array).

Perhaps something like this: https://stackoverflow.com/a/70694878

It seems it still needs specifying all union type values to be passed again to the util function as per the comment. I will leave it to another PR/issue to unblock this PR since the query parameter cannot be invalid from UI events unless user edits the URL intentionally or has a typo. It looks like a common request. Thanks for the pointers @bbovenzi .

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Oct 8, 2024

Slightly off-topic but I see PRs with only new UI folder changes running tests for databases like mysql, postgres and sqlite which takes 20 minutes each along with integration tests, docs builds (--docs-only and spellcheck take 20+ mins) which I also guess is not required. Skipping them can speed up UI PRs build time and also reduce CI load. I tried to figure out how to skip them which should probably be done in selective_check function in ci_commands.py but couldn't figure out the exact conditional and logic.

I believe those are part of 'default' tests. For instance if we check the tests-sqlite, we see that they require run-tests and this is basically:

    @cached_property
    def run_tests(self) -> bool:
        return self._should_be_run(FileGroupForCi.ALL_SOURCE_FILES)

So they will run every time.

Maybe the CI team has an idea on how we can improve that, and do different 'default tests' depending on the Front-end and backend. (if front files are touch run default front-end, if .py file are run -> run default pytest)

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Oct 8, 2024

cc: @potiuk Just in case you have an idea.

@bbovenzi bbovenzi merged commit 9c4b81d into apache:main Oct 9, 2024
52 checks passed
kunaljubce pushed a commit to kunaljubce/airflow that referenced this pull request Oct 13, 2024
* Add support to filter by last dagrun state in UI.

* Fix lint errors.

* Fix lint errors.

* Fix PR comments over null checks and query parameter to be last_dag_run_state.
pavansharma36 pushed a commit to pavansharma36/airflow that referenced this pull request Oct 14, 2024
* Add support to filter by last dagrun state in UI.

* Fix lint errors.

* Fix lint errors.

* Fix PR comments over null checks and query parameter to be last_dag_run_state.
R7L208 pushed a commit to R7L208/airflow that referenced this pull request Oct 17, 2024
* Add support to filter by last dagrun state in UI.

* Fix lint errors.

* Fix lint errors.

* Fix PR comments over null checks and query parameter to be last_dag_run_state.
harjeevanmaan pushed a commit to harjeevanmaan/airflow that referenced this pull request Oct 23, 2024
* Add support to filter by last dagrun state in UI.

* Fix lint errors.

* Fix lint errors.

* Fix PR comments over null checks and query parameter to be last_dag_run_state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AIP-38 | DAGs List | Filter by last_dag_run_state
3 participants