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

Migrate Edge calls for Worker to FastAPI part 1 - Worker routes #44311

Merged

Conversation

jscheffl
Copy link
Contributor

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.

@jscheffl jscheffl added area:API Airflow's REST/HTTP API AIP-69 Edge Executor provider:edge Edge Executor / Worker (AIP-69) labels Nov 23, 2024
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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_active should 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.")

@jscheffl jscheffl force-pushed the feature/separate-edge-api-from-internal-api branch 3 times, most recently from 0bb863f to 6f569b7 Compare November 24, 2024 17:04
@jscheffl jscheffl changed the title Migrate Edge calls for Worker to FastAPI Migrate Edge calls for Worker to FastAPI part 1 - Worker routes Nov 24, 2024
@jscheffl jscheffl force-pushed the feature/separate-edge-api-from-internal-api branch from 6f569b7 to 88d5071 Compare November 27, 2024 21:31
@jscheffl jscheffl requested a review from kaxil November 27, 2024 22:39
@jscheffl jscheffl force-pushed the feature/separate-edge-api-from-internal-api branch from 53f388d to 7c88e11 Compare November 28, 2024 22:31
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

One "bigger" thing about revealing too much for auth errors.

@jscheffl
Copy link
Contributor Author

One "bigger" thing about revealing too much for auth errors.

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...)

@jscheffl jscheffl force-pushed the feature/separate-edge-api-from-internal-api branch from 335c035 to 4080945 Compare November 30, 2024 14:51
@jscheffl
Copy link
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...)

@potiuk
Copy link
Member

potiuk commented Nov 30, 2024

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

@jscheffl jscheffl merged commit 6057a2e into apache:main Nov 30, 2024
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-69 Edge Executor area:API Airflow's REST/HTTP API area:providers provider:edge Edge Executor / Worker (AIP-69)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants