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 3 - Jobs routes #44433

Merged

Conversation

jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Nov 27, 2024

Follow-up PR as incremental part of #44311 and #44330

Note: Only the last commit is the relevant change, the first 4 commits are from #44311 and #44330

To prepare EdgeWorker to be independent of AIP-44 Internal API, this PR is the third step in adding/migrating to FastAPI. The calls to "Jobs" API to (1) fetch a job and (2) report job 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 cause for ongoing Airflow 2.10 Connexion API + Swagger manually need to be generated whereas the main workstream for Airflow 3 uses FastAPI.

Choose a reason for hiding this comment

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

Copilot reviewed 24 out of 24 changed files in this pull request and generated no suggestions.

@jscheffl jscheffl added AIP-69 Edge Executor area:API Airflow's REST/HTTP API labels Nov 27, 2024
@jscheffl jscheffl force-pushed the feature/separate-edge-api-from-internal-api-3 branch 2 times, most recently from 6cc299b to a7df286 Compare November 28, 2024 23:06
@potiuk
Copy link
Member

potiuk commented Nov 29, 2024

I will wait for 1 and 2 to be merged :)

@jscheffl
Copy link
Contributor Author

jscheffl commented Nov 29, 2024

I will wait for 1 and 2 to be merged :)

As internal API is now "gone" I will have a hard time getting #1+#2 merged. As there are some merge conflicts as well I'll focus on PR#3 + #4...

But anyway let me resolve the conflicts and integrate the feedback from Kaxil then it might be cleaner already.

UPDATE: Okay now I realize that the internal API is gone but all the previous functions are moved to provider package... need to catch-up coming home...

@potiuk
Copy link
Member

potiuk commented Nov 29, 2024

UPDATE: Okay now I realize that the internal API is gone but all the previous functions are moved to provider package... need to catch-up coming home...

👀

@jscheffl jscheffl force-pushed the feature/separate-edge-api-from-internal-api-3 branch 2 times, most recently from 1a0a928 to 2fc3464 Compare November 30, 2024 22:23
@jscheffl
Copy link
Contributor Author

I will wait for 1 and 2 to be merged :)

You just merged 2 after I merged 1... so "ball is in your field" now :-D

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 11 changed files in this pull request and generated 1 suggestion.

Files not reviewed (6)
  • 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/provider.yaml: 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
Comments skipped due to low confidence (1)

providers/src/airflow/providers/edge/cli/edge_command.py:89

  • The word 'AIrflow' should be 'Airflow'.
                "Error: EdgeWorker is currently broken on Airflow 3/main due to removal of AIP-44, rework for AIP-72."

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.

NICE... Way easier to review once 1 and 2 merged :)

@jscheffl jscheffl force-pushed the feature/separate-edge-api-from-internal-api-3 branch from 2fc3464 to e30aa57 Compare December 1, 2024 09:26
@jscheffl jscheffl merged commit 161beeb into apache:main Dec 1, 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 kind:documentation provider:edge Edge Executor / Worker (AIP-69)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants