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

AIP-84 Add Lists Jobs with Filters API #43859

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jason810496
Copy link
Contributor

Closes: #43660
Related: #43657

Feature

I’ve placed the endpoint in airflow/api_fastapi/core_api/routes/public/job.py, but I’m not certain if this is the most appropriate location—perhaps airflow/api_fastapi/core_api/routes/cli/job.py would also fine ?
The query parameters, response data models, and implementation are consistent with the rest of the public API, utilizing common.parameters and paginated_select.

Tests

The test cases are refactored from the original airflow/tests/cli/commands/test_jobs_command.py, as the CLI should yield the same results when leveraging the API instead of direct DB access.

Question

Hi @bugraoz93, I’m curious about the distinction between this PR’s endpoint (List Jobs with filters) and #43661 (List Jobs), as both endpoints seem to provide job listings. From my perspective, calling this "List Jobs with filters" endpoint without query parameters would yield the same response as the "List Jobs" endpoint.
cc @josix

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Nov 10, 2024
@jason810496 jason810496 changed the title AIP-81 Add Lists Jobs with filters API AIP-81 Add Lists Jobs with Filters API Nov 10, 2024
@bugraoz93
Copy link
Collaborator

Hey @jason810496,

That’s a good question, and I’m glad you raised it! @rawwar and I had a similar discussion, and it seems we all have similar concerns.

At first, I thought keeping "List Jobs" and "List Jobs with Filters" as separate endpoints made sense for clarity. But after thinking it through more and taking a closer look, I believe combining them into a single "List Jobs" endpoint with optional query parameters for filters might be a better approach. This way, the endpoint would return all jobs by default, with filtering options (as a QueryParameter) as needed. It would make it both straightforward and easy to maintain.

Thanks for raising this! I am happy to keep the discussion going if others have thoughts on this approach. Otherwise, let’s go ahead and merge it into one endpoint.

cc: @josix @rawwar

@bugraoz93
Copy link
Collaborator

I’ve placed the endpoint in airflow/api_fastapi/core_api/routes/public/job.py, but I’m not certain if this is the most appropriate location—perhaps airflow/api_fastapi/core_api/routes/cli/job.py would also fine ?
The query parameters, response data models, and implementation are consistent with the rest of the public API, utilizing common.parameters and paginated_select.

@jason810496 I missed including this part in my previous answer. I responded from my mind but forgot to put it into text. :)

I think it makes more sense to keep the endpoint under Core API. Since most CLI calls will still rely on Core API, creating a separate category for just a few endpoints could add unnecessary complexity. By sticking to Core API, we stay consistent with the rest of the API and follow the same rules across the board. This way, we avoid extra maintenance overhead and keep things simple as we scale.

@bbovenzi bbovenzi changed the title AIP-81 Add Lists Jobs with Filters API AIP-84 Add Lists Jobs with Filters API Nov 11, 2024
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.

Nice, looking good overall.

A couple of suggestions and we can merge.

airflow/api_fastapi/core_api/routes/public/job.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_job.py Outdated Show resolved Hide resolved
@jason810496 jason810496 force-pushed the feature/AIP-81/add-lists-jobs-with-filter-api branch from 03a5681 to e23c805 Compare November 16, 2024 06:36
@jason810496 jason810496 force-pushed the feature/AIP-81/add-lists-jobs-with-filter-api branch from e23c805 to 37c1f8d Compare November 18, 2024 13:03
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.

Tests need fixing, but looking god to me.

tests_common/test_utils/format_datetime.py Show resolved Hide resolved
Copy link
Collaborator

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the changes!

@jason810496 jason810496 force-pushed the feature/AIP-81/add-lists-jobs-with-filter-api branch from 37c1f8d to 0df5e51 Compare November 20, 2024 12:37
@pierrejeambrun
Copy link
Member

I think rebasing went wrong on the genrated file part.

When you have any conflicts on those (openapi spec, or front end generated code). You can just re-run manually the hooks to refresh the files:

pre-commit run generate-openapi-spec --all-files
pre-commit run ts-compile-format-lint-ui --all-files

@jason810496 jason810496 force-pushed the feature/AIP-81/add-lists-jobs-with-filter-api branch from 76440f1 to ba9f034 Compare November 21, 2024 13:09
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.

Nice one minor thing need to be adjusted, then we can merge.

airflow/api_fastapi/core_api/routes/public/job.py Outdated Show resolved Hide resolved
tests_common/test_utils/format_datetime.py Show resolved Hide resolved
@jason810496 jason810496 force-pushed the feature/AIP-81/add-lists-jobs-with-filter-api branch from ba9f034 to 1f25c7b Compare November 22, 2024 07:51
@jason810496
Copy link
Contributor Author

Just fix the trailing endpoint naming.
I will work on follow up PR to refactor the test case with manual replace to datetime_zulu_format utility function 👍

@pierrejeambrun
Copy link
Member

Well I just tried the conflict resolution via the github interface. (because it was a really small conflict), quite handy for quick and simple conflicts.

@jason810496
Copy link
Contributor Author

Hi @pierrejeambrun , I have ran the pre-commit locally and didn't not encounter current CI issue, not sure about what cause the Generate the FastAPI API spec error.

@pierrejeambrun
Copy link
Member

This is due to the recent pydantic release that breaks some stuff. Main lower bound has been added and should be better, you most likely not have the appropriate pydantic version locally. Now it should be >= 2.10.1(#44284)

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 22, 2024

Most likely the CI image used to run the pre-commit hook (in_container script) still has an old pydantic version.

You can run:

breeze ci-image build --python 3.9

To refresh the image. And you should get the appropriate generated spec locally then.

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-81 Implement List Jobs with Filter Endpoint in FastAPI
4 participants