Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
shahar1 commented on PR #60394: URL: https://github.com/apache/airflow/pull/60394#issuecomment-3848316777 > Hello @shahar1! > This PR broke the deferrable mode for `CloudRunExecuteJobOperator` could you revert this PR? > Here is the screenshot from the last run of the system test: > https://github.com/user-attachments/assets/25265678-dc1c-45d3-a882-7d7e4a86828c"; /> > Thanks for informing, I'll take care of it later on - I had merged it before our discussion regarding the system tests. As I'm also the the release manager of the next provider wave, I'll ensure that it's get reverted by then. I'll be happy to discuss with your team regarding the integration of the system tests to the repo's CI, so we could run specific system twsts before merging / getting alerted on Airflow's slack channel. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
MaksYermak commented on PR #60394: URL: https://github.com/apache/airflow/pull/60394#issuecomment-3848132514 Hello @shahar1! This PR broke the deferrable mode for `CloudRunExecuteJobOperator` could you revert this PR? Here is the screenshot from the last run of the system test: https://github.com/user-attachments/assets/25265678-dc1c-45d3-a882-7d7e4a86828c"; /> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
shahar1 merged PR #60394: URL: https://github.com/apache/airflow/pull/60394 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
arjav1528 commented on PR #60394: URL: https://github.com/apache/airflow/pull/60394#issuecomment-3800510790 @uranusjr could you please approve the changes and merge the PR -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
arjav1528 commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2724462588 ## providers/google/src/airflow/providers/google/cloud/triggers/cloud_run.py: ## @@ -71,6 +74,7 @@ def __init__( impersonation_chain: str | Sequence[str] | None = None, polling_period_seconds: float = 10, timeout: float | None = None, +transport: Literal["rest", "grpc"] | None = "grpc", Review Comment: You're right. The default should be None to match CloudRunHook, CloudRunAsyncHook, and CloudRunExecuteJobOperator, which all default to None. This allows the Google client library to auto-select the transport consistently in both sync and deferrable modes. I'll change line 77 to transport: Literal["rest", "grpc"] | None = None and simplify _get_async_hook() to just pass self.transport directly without the None-to-grpc conversion. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
shahar1 commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2724260370 ## providers/google/src/airflow/providers/google/cloud/triggers/cloud_run.py: ## @@ -71,6 +74,7 @@ def __init__( impersonation_chain: str | Sequence[str] | None = None, polling_period_seconds: float = 10, timeout: float | None = None, +transport: Literal["rest", "grpc"] | None = "grpc", Review Comment: Shouldn't this be `None` like the previous one for consistent behavior? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
arjav1528 commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2722957407 ## providers/google/src/airflow/providers/google/cloud/triggers/cloud_run.py: ## @@ -71,6 +74,7 @@ def __init__( impersonation_chain: str | Sequence[str] | None = None, polling_period_seconds: float = 10, timeout: float | None = None, +transport: str | None = None, Review Comment: @uranusjr could you review the changes and merge if good to go -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
arjav1528 commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2706043769 ## providers/google/tests/unit/google/cloud/hooks/test_cloud_run.py: ## @@ -259,6 +259,36 @@ def test_delete_job(self, mock_batch_service_client, cloud_run_hook): cloud_run_hook.delete_job(job_name=JOB_NAME, region=REGION, project_id=PROJECT_ID) cloud_run_hook._client.delete_job.assert_called_once_with(delete_request) [email protected]( + "airflow.providers.google.common.hooks.base_google.GoogleBaseHook.__init__", +new=mock_base_gcp_hook_default_project_id, +) [email protected]("airflow.providers.google.cloud.hooks.cloud_run.JobsClient") +def test_get_conn_with_transport(self, mock_jobs_client): +"""Test that transport parameter is passed to JobsClient.""" +hook = CloudRunHook(transport="rest") +hook.get_credentials = self.dummy_get_credentials +hook.get_conn() + +mock_jobs_client.assert_called_once() +call_kwargs = mock_jobs_client.call_args[1] +assert call_kwargs["transport"] == "rest" + [email protected]( + "airflow.providers.google.common.hooks.base_google.GoogleBaseHook.__init__", +new=mock_base_gcp_hook_default_project_id, +) [email protected]("airflow.providers.google.cloud.hooks.cloud_run.JobsClient") +def test_get_conn_without_transport(self, mock_jobs_client): +"""Test that JobsClient is created with default 'grpc' transport when not specified.""" +hook = CloudRunHook() +hook.get_credentials = self.dummy_get_credentials +hook.get_conn() + +mock_jobs_client.assert_called_once() +call_kwargs = mock_jobs_client.call_args[1] +assert call_kwargs["transport"] == "grpc" Review Comment: I. have applied all the suggested changes, do review the PR at your availability -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
olegkachur-e commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2686367899 ## providers/google/src/airflow/providers/google/cloud/hooks/cloud_run.py: ## @@ -67,16 +67,21 @@ class CloudRunHook(GoogleBaseHook): If set as a sequence, the identities from the list must grant Service Account Token Creator IAM role to the directly preceding identity, with first account from the list granting this role to the originating account. +:param transport: Optional. The transport to use for API requests. Can be 'rest' or 'grpc'. Review Comment: In the [docs](https://docs.cloud.google.com/python/docs/reference/run/latest/google.cloud.run_v2.services.jobs.JobsClient#google_cloud_run_v2_services_jobs_JobsClient) it is referenced as: > "Optional[Union[str,JobsTransport,Callable[..., JobsTransport]]] > The transport to use, or a Callable that constructs and returns a new transport. If a Callable is given, it will be called with the same set of initialization arguments as used in the JobsTransport constructor. If set to None, a transport is chosen automatically. I think it makes sense to replicate similar hook behavior, at least to some extent, like `transport: Literal['rest', 'grpc'] | None = None)` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
olegkachur-e commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2686367899 ## providers/google/src/airflow/providers/google/cloud/hooks/cloud_run.py: ## @@ -67,16 +67,21 @@ class CloudRunHook(GoogleBaseHook): If set as a sequence, the identities from the list must grant Service Account Token Creator IAM role to the directly preceding identity, with first account from the list granting this role to the originating account. +:param transport: Optional. The transport to use for API requests. Can be 'rest' or 'grpc'. Review Comment: In the docs it is referenced as: > "Optional[Union[str,JobsTransport,Callable[..., JobsTransport]]] > The transport to use, or a Callable that constructs and returns a new transport. If a Callable is given, it will be called with the same set of initialization arguments as used in the JobsTransport constructor. If set to None, a transport is chosen automatically. I think it makes sense to replicate similar hook behavior, at least to some extent, like `transport: Literal['rest', 'grpc'] | None = None)` ## providers/google/tests/unit/google/cloud/hooks/test_cloud_run.py: ## @@ -259,6 +259,36 @@ def test_delete_job(self, mock_batch_service_client, cloud_run_hook): cloud_run_hook.delete_job(job_name=JOB_NAME, region=REGION, project_id=PROJECT_ID) cloud_run_hook._client.delete_job.assert_called_once_with(delete_request) [email protected]( + "airflow.providers.google.common.hooks.base_google.GoogleBaseHook.__init__", +new=mock_base_gcp_hook_default_project_id, +) [email protected]("airflow.providers.google.cloud.hooks.cloud_run.JobsClient") +def test_get_conn_with_transport(self, mock_jobs_client): +"""Test that transport parameter is passed to JobsClient.""" +hook = CloudRunHook(transport="rest") +hook.get_credentials = self.dummy_get_credentials +hook.get_conn() + +mock_jobs_client.assert_called_once() +call_kwargs = mock_jobs_client.call_args[1] +assert call_kwargs["transport"] == "rest" + [email protected]( + "airflow.providers.google.common.hooks.base_google.GoogleBaseHook.__init__", +new=mock_base_gcp_hook_default_project_id, +) [email protected]("airflow.providers.google.cloud.hooks.cloud_run.JobsClient") +def test_get_conn_without_transport(self, mock_jobs_client): +"""Test that JobsClient is created with default 'grpc' transport when not specified.""" +hook = CloudRunHook() +hook.get_credentials = self.dummy_get_credentials +hook.get_conn() + +mock_jobs_client.assert_called_once() +call_kwargs = mock_jobs_client.call_args[1] +assert call_kwargs["transport"] == "grpc" Review Comment: Both `test_get_conn_with_transport` and `test_get_conn_without_transport` looks pretty similar, is there a chance to combine them with `@pytest.mark.parametrize` ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
arjav1528 commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2681310653 ## providers/google/src/airflow/providers/google/cloud/triggers/cloud_run.py: ## @@ -71,6 +74,7 @@ def __init__( impersonation_chain: str | Sequence[str] | None = None, polling_period_seconds: float = 10, timeout: float | None = None, +transport: str | None = None, Review Comment: I have updated the code, you can check now -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
arjav1528 commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2681288551 ## providers/google/src/airflow/providers/google/cloud/triggers/cloud_run.py: ## @@ -71,6 +74,7 @@ def __init__( impersonation_chain: str | Sequence[str] | None = None, polling_period_seconds: float = 10, timeout: float | None = None, +transport: str | None = None, Review Comment: Yes, the transport options are static. Using Literal["rest", "grpc"] is appropriate. my bad, I didnt notice is properly -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add transport parameter to CloudRunHook and CloudRunExecuteJobOperator [airflow]
uranusjr commented on code in PR #60394: URL: https://github.com/apache/airflow/pull/60394#discussion_r2681036663 ## providers/google/src/airflow/providers/google/cloud/triggers/cloud_run.py: ## @@ -71,6 +74,7 @@ def __init__( impersonation_chain: str | Sequence[str] | None = None, polling_period_seconds: float = 10, timeout: float | None = None, +transport: str | None = None, Review Comment: If this defaults to "grpc", why not just do `transport: str = "grpc"`? Also it might be a good idea to use Literal here? I’m not sure if the transports supported by these operators a static or not. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
