Migrate Edge calls for Worker to FastAPI part 1 - Worker routes#44311
Merged
jscheffl merged 1 commit intoapache:mainfrom Nov 30, 2024
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot reviewed 5 out of 18 changed files in this pull request and generated 3 suggestions.
Files not reviewed (13)
- providers/src/airflow/providers/edge/CHANGELOG.rst: Language not supported
- providers/src/airflow/providers/edge/openapi/edge_worker_api_v1.yaml: Language not supported
- providers/src/airflow/providers/edge/cli/api_client.py: Evaluated as low risk
- providers/src/airflow/providers/edge/cli/edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/executors/edge_executor.py: Evaluated as low risk
- providers/src/airflow/providers/edge/provider.yaml: Evaluated as low risk
- providers/tests/edge/worker_api/routes/test_worker.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_worker.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/rpc_api.py: Evaluated as low risk
- providers/tests/edge/worker_api/routes/test_rpc_api.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/app.py: Evaluated as low risk
- providers/tests/edge/cli/test_edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/_v2_compat.py: Evaluated as low risk
Comments skipped due to low confidence (1)
providers/src/airflow/providers/edge/worker_api/datamodels.py:45
- The description for
jobs_activeshould be updated to clarify that it is the number of active jobs the worker is running.
jobs_active: int = Field(0, description="Number of active jobs the worker is running.")
providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py
Outdated
Show resolved
Hide resolved
providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py
Outdated
Show resolved
Hide resolved
providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py
Outdated
Show resolved
Hide resolved
0bb863f to
6f569b7
Compare
6f569b7 to
88d5071
Compare
53f388d to
7c88e11
Compare
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
kaxil
reviewed
Nov 29, 2024
providers/src/airflow/providers/edge/worker_api/routes/worker.py
Outdated
Show resolved
Hide resolved
kaxil
reviewed
Nov 29, 2024
providers/src/airflow/providers/edge/worker_api/routes/worker.py
Outdated
Show resolved
Hide resolved
kaxil
approved these changes
Nov 29, 2024
potiuk
reviewed
Nov 29, 2024
potiuk
approved these changes
Nov 29, 2024
Member
potiuk
left a comment
There was a problem hiding this comment.
One "bigger" thing about revealing too much for auth errors.
Contributor
Author
Point taken. Thanks for the review. Actually it was a take-over from existing internal API. Would we need to harden this as well before we do a 2.10.4? See https://github.com/apache/airflow/blob/2.10.3/airflow/api_internal/endpoints/rpc_api_endpoint.py#L190 (now removed on main...) |
335c035 to
4080945
Compare
Contributor
Author
|
@kaxil / @potiuk Thanks for the review! All things adjusted... but as AIP-44 needed to re-work a lot I needed to fully re-base and restore AIP-44 broken function. As Airflow 3 is now broken... after this PR v2.10 is working again. Do you want to have a second round or good to merge as is? (And follow-ups will be taken care...) |
Member
|
I had a quick look - I am good to go. I think we need to align on the near future strategy for breaking/non-breaking strategy for the edge worker - see #44494 (comment) but this one is good to go I think |
potiuk
reviewed
Nov 30, 2024
got686-yandex
pushed a commit
to got686-yandex/airflow
that referenced
this pull request
Jan 30, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To prepare EdgeWorker to be independent of AIP-44 Internal API, this PR is the second step in adding/migrating to FastAPI. The calls to "Worker" API to (1) register a worker and (2) set the state are now real REST API calls, not using internal API.
I would separate the other internal API calls to follow-up PRs as this is already quite large. Especially ecause for ongoing Airflow 2.10 Connexion API + Swagger manually need to be generated whereas the main workstram for Airflow 3 uses FastAPI.