osintalex commented on code in PR #70679:
URL: https://github.com/apache/airflow/pull/70679#discussion_r3681878916
##########
providers/google/src/airflow/providers/google/cloud/operators/vertex_ai/batch_prediction_job.py:
##########
@@ -159,7 +159,18 @@ class
CreateBatchPredictionJobOperator(GoogleCloudBaseOperator):
:param poll_interval: Interval size which defines how often job status is
checked in deferrable mode.
"""
- template_fields = ("region", "project_id", "model_name",
"impersonation_chain", "job_display_name")
+ template_fields = (
+ "region",
+ "project_id",
+ "model_name",
+ "impersonation_chain",
+ "job_display_name",
+ "gcs_source",
+ "bigquery_source",
+ "gcs_destination_prefix",
+ "gcs_destination_prefix",
Review Comment:
Ty will catch.
Regarding this https://github.com/apache/airflow/issues/70296
I don't think I really understand the implications of this. From what I can
tell in the operator the `__init__` method simply assigns these attributes onto
the class
https://github.com/apache/airflow/blob/main/providers/google/src/airflow/providers/google/cloud/operators/vertex_ai/batch_prediction_job.py#L165.
I'm struggling to see how that would cause some sort of issue or antipattern
here, it doesn't look like any anything happens in the constructor here apart
from the assignment, which I believe is in alignment with your guide here
https://airflow.apache.org/docs/apache-airflow/stable/howto/custom-operator.html#limitations.
Let me know if I've misunderstood though.
As for 2, yes I ran into this in production when running a batch inference
job with sources in bigquery and destination in bigquery. It made in my
experience the operator hard to understand and use since the bigquery source
and destinations were not templatable, which I found counterintuitive. By
comparison these sorts of fields are typically templatable across airflow, e.g.
on bigquery insert job operators. It was also very difficult to debug since the
job just mysteriously fails and you don't get any error feedback on the
source/destination being invalid.
This is what I was trying to articulate by 'normal', i.e. allowing these
fields to be templatable in my opinion is much more consistent with most other
airflow operators, where source/destination outputs to GCS or BQ are typically
templatable.
Lacking the ability to template the operator makes it, in my opinion, quite
unusable in most professional production environments where you would have
dev/staging/prod environments that all need different input/output
configurations but should run the same code.
While I could create a new class that inherits from it, that doesn't feel so
good to me since I would argue most people using this operator in production
systems will want the same thing as me, i.e. ability to template these fields
dynamically based on environment. Seems to me like something which should be
supported by default.
--
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]
