AIP-84 Migrate Trigger Dag Run endpoint to FastAPI #43875
Conversation
boring-cyborg
bot
added
area:API
rawwar
force-pushed
the
kalyan/AIP-84/trigger_dag_run
branch
from
b11977f
to
ed49619
Compare
| """Trigger DAG Run Serializer for POST body.""" | ||
|
|
||
| dag_run_id: str | None | ||
| logical_date: datetime |
There was a problem hiding this comment.
According to legacy endpoint implementation, logical_date can't be None. So, I had to remove datetime| None.
There was a problem hiding this comment.
I think we want to remove this entirely and let the backend interpret the logical_date. I assume that has to happen at a deeper layer than use fastapi is that right @uranusjr ?
There was a problem hiding this comment.
I referred to the implementation and updated this PR
There was a problem hiding this comment.
Yeah a deeper layer should infer logical date from the data interval, or use now() if those are not provided. This should not take the argument at all.
…AIP-84/trigger_dag_run
…AIP-84/trigger_dag_run
|
|
||
| # Mypy issue https://github.com/python/mypy/issues/1362 | ||
| @computed_field # type: ignore[misc] | ||
| @property | ||
| def _logical_date(self) -> datetime: | ||
| return self.logical_date or timezone.utcnow() |
There was a problem hiding this comment.
Following @uranusjr, we remove everything related to logical_date (if the db allows us to insert like this), and we will leave a deeper layer populate that for us. But then without a logical_date we need to manually create the dag_run_id for now.
| @model_validator(mode="after") | ||
| def validate_dag_run_id(self): | ||
| if not self.dag_run_id: | ||
| self.dag_run_id = DagRun.generate_run_id(DagRunType.MANUAL, self.logical_date) |
There was a problem hiding this comment.
@uranusjr If we remove the logical_date, then we have None for logical date when creating the dagrun with create_dagrun, and unfortunately it needs type + logical_date to infer the run_id. This is why we need to manually fill the run_id here in case it's not there, but I am not a big fan of it. (is that ok ?)
I assume this will be updated later when the logical_date change will take place => create_dagrun will be able to generate an appropriate run_id without providing logical_date ?
There was a problem hiding this comment.
For now, Shall I just remove logical_date from request body and use current time by default to generate run_id? Once updated made to create_dagrun, this can also be updated
There was a problem hiding this comment.
Yes, let's remove it from the request body.
rawwar
requested review from
ryanahamilton,
jscheffl and
ephraimbuddy
as code owners
…AIP-84/trigger_dag_run
| conf: dict = Field(default_factory=dict) | ||
| note: str | None = None | ||
|
|
||
| model_config = {"extra": "forbid"} |
There was a problem hiding this comment.
I suppose it'll be great to keep this consistent across all APIs. Legacy API throws 400 bad request on extra properties passed, while since FastAPI and Pydantic ignore extra properties, new APIs don't throw an error yet.
We could either forbid extra properties for all models or for none of them, so the users have a consistent view of the APIs. In new APIs, I suppose currently forbidding is being done only in CreateAssetEventsBody here.
There was a problem hiding this comment.
Yeah. I created a draft to start a conversation on this: #44306
Forgot to link this
related to #42701