You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "hterik (via GitHub)" <gi...@apache.org> on 2023/10/12 12:22:04 UTC
[PR] Base date for fetching dag grid view must include selected run_id [airflow]
hterik opened a new pull request, #34887:
URL: https://github.com/apache/airflow/pull/34887
Previously, if user set dag_run_id parameter in the url, that refers to a old run, which doesn't fit in the most recent 25 runs, then the requested run will not be selected.
This change fixes this by setting the base_date to a time where the run_id is known to exist if dag_run_id is provided as an explicit query parameter.
closes: #34723
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "sinwoobang (via GitHub)" <gi...@apache.org>.
sinwoobang commented on code in PR #34887:
URL: https://github.com/apache/airflow/pull/34887#discussion_r1392931525
##########
airflow/www/views.py:
##########
@@ -3467,25 +3468,44 @@ def grid_data(self):
if num_runs is None:
num_runs = conf.getint("webserver", "default_dag_run_display_number")
- try:
- base_date = timezone.parse(request.args["base_date"], strict=True)
- except (KeyError, ValueError):
- base_date = dag.get_latest_execution_date() or timezone.utcnow()
+ dagrun = None
+ if run_id:
+ with create_session() as session:
+ dagrun = dag.get_dagrun(run_id=run_id, session=session)
+ if not dagrun:
+ return {"error": f"can't find dag_run_id={run_id}"}, 404
+ base_date = dagrun.execution_date
+ else:
+ try:
+ base_date = timezone.parse(request.args["base_date"], strict=True)
+ except (KeyError, ValueError):
+ base_date = dag.get_latest_execution_date() or timezone.utcnow()
with create_session() as session:
query = select(DagRun).where(DagRun.dag_id == dag.dag_id, DagRun.execution_date <= base_date)
run_type = request.args.get("run_type")
if run_type:
+ if run_id:
+ return {"error": "Can not provide filters when dag_run_id filter is selected."}, 400
query = query.where(DagRun.run_type == run_type)
run_state = request.args.get("run_state")
if run_state:
+ if run_id:
+ return {"error": "Can not provide filters when dag_run_id filter is selected."}, 400
query = query.where(DagRun.state == run_state)
dag_runs = wwwutils.sorted_dag_runs(
query, ordering=dag.timetable.run_ordering, limit=num_runs, session=session
)
+ if dagrun:
+ found_requested_run_id = any(True for d in dag_runs if d.run_id == run_id)
Review Comment:
@hterik It makes sense. You are correct :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1832384351
> Is it possible to take the base date changes separately as an improvement? This bug makes the most recent versions of airflow completely unusable for us, would be great to have some kind of fix in asap. The situation where one navigates from old run to new run isn't even possible to reach today due to this bug so i don't think we are regressing anything? Maybe i'm missing something, can you explain the scenario a bit more in detail? Does this change break the base_date selector in any other way?
Sure I can do a follow up PR. Let's still rebase.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1890961758
There's conflicts
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #34887:
URL: https://github.com/apache/airflow/pull/34887#discussion_r1356736539
##########
airflow/www/views.py:
##########
@@ -3465,25 +3466,42 @@ def grid_data(self):
if num_runs is None:
num_runs = conf.getint("webserver", "default_dag_run_display_number")
- try:
- base_date = timezone.parse(request.args["base_date"], strict=True)
- except (KeyError, ValueError):
- base_date = dag.get_latest_execution_date() or timezone.utcnow()
+ dagrun = None
+ if run_id:
+ with create_session() as session:
+ dagrun = dag.get_dagrun(run_id=run_id, session=session)
+ if not dagrun:
+ return {"error": f"can't find dag_run_id={run_id}"}, 404
+ base_date = dagrun.execution_date
Review Comment:
The old code in `get_date_time_num_runs_dag_runs_form_data` that does similar lookups has following workaround, is that still needed here also?
# The DateTimeField widget truncates milliseconds and would loose
# the first dag run. Round to next second.
base_date = (date_time + datetime.timedelta(seconds=1)).replace(microsecond=0)
I didn't see any such issue during my short exploratory testing of this.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "jens-scheffler-bosch (via GitHub)" <gi...@apache.org>.
jens-scheffler-bosch commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1787550600
I tried my best to review this PR but I was not able to get the UI running with breeze locally. GRID view did not show up anymore. Unfortunately I'm not really a TypeScript developer (noop level) and could not see the root cause.
Reading the code I fear a bit that most of the logic is made in backend in views.py. As already above I am a noop but would assume it is better to have the logic in the client side, similar like "If requested run_id not in runs loaded, then load an are where the run_id is contained.
But I'd leave a review to one of the UX experts... @pierrejeambrun / @bbovenzi
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "sinwoobang (via GitHub)" <gi...@apache.org>.
sinwoobang commented on code in PR #34887:
URL: https://github.com/apache/airflow/pull/34887#discussion_r1392614238
##########
airflow/www/views.py:
##########
@@ -3467,25 +3468,44 @@ def grid_data(self):
if num_runs is None:
num_runs = conf.getint("webserver", "default_dag_run_display_number")
- try:
- base_date = timezone.parse(request.args["base_date"], strict=True)
- except (KeyError, ValueError):
- base_date = dag.get_latest_execution_date() or timezone.utcnow()
+ dagrun = None
+ if run_id:
+ with create_session() as session:
+ dagrun = dag.get_dagrun(run_id=run_id, session=session)
+ if not dagrun:
+ return {"error": f"can't find dag_run_id={run_id}"}, 404
+ base_date = dagrun.execution_date
+ else:
+ try:
+ base_date = timezone.parse(request.args["base_date"], strict=True)
+ except (KeyError, ValueError):
+ base_date = dag.get_latest_execution_date() or timezone.utcnow()
with create_session() as session:
query = select(DagRun).where(DagRun.dag_id == dag.dag_id, DagRun.execution_date <= base_date)
run_type = request.args.get("run_type")
if run_type:
+ if run_id:
+ return {"error": "Can not provide filters when dag_run_id filter is selected."}, 400
query = query.where(DagRun.run_type == run_type)
run_state = request.args.get("run_state")
if run_state:
+ if run_id:
+ return {"error": "Can not provide filters when dag_run_id filter is selected."}, 400
query = query.where(DagRun.state == run_state)
dag_runs = wwwutils.sorted_dag_runs(
query, ordering=dag.timetable.run_ordering, limit=num_runs, session=session
)
+ if dagrun:
+ found_requested_run_id = any(True for d in dag_runs if d.run_id == run_id)
Review Comment:
Would it be better to execute a query directly instead of using a Python checker? I assume having a Dag that has over 10,000 runs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1833492307
Rebased now.
I see the problem with base date now. The whole base date selector and the filter options stop working if a `dag_run_id` was present in the URL (though `dag_run_id` in url didn't work at all before so questionable if this is a regression).
I did a quick and dirty attempt of fixing it but couldn't find a good solution in short time.
Something like below did make both the selector and Clear filters button work. Except if one clicks the current date, because when base date is not present it assumes it is the same time. It feels like i'm slowly migrating essentially the whole useFilters and useSelection to the Main function, it's a bit out of my comfort zone to be making such big changes here. I also don't have much time to look deeper into this in the close future. If you feel like you have better control over this i would happy to hand it over.
```
const Main = () => {
const [searchParams, setSearchParams] = useSearchParams();
const [firstRunIdSetByUrl, setFirstRunIdSetByUrl] = useState(searchParams.get(RUN_ID));
const [prevBasedate, setPrevBasedate] = useState(searchParams.get(BASE_DATE_PARAM));
const baseDate = searchParams.get(BASE_DATE_PARAM);
if (baseDate != prevBasedate) {
setPrevBasedate(baseDate)
setFirstRunIdSetByUrl(null)
searchParams.delete("dag_run_id")
searchParams.delete("task_id")
setSearchParams(searchParams);
}
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1811942748
@ryanahamilton @ashb @bbovenzi @pierrejeambrun can you please help in reviewing or find someone else who can review this?
This has been pending for some time now and fixes a very annoying bug where one can not permalink to individual dagruns https://github.com/apache/airflow/issues/34723
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1948287425
@hterik Can you handle the rebase and resolve conflicts?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi merged PR #34887:
URL: https://github.com/apache/airflow/pull/34887
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1953238610
Yeah, would be great - as this fix was pushed multiple times... next release will be cut in ~24h - if rebased and reviewed the chance is likely to get it in...
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1819160336
> > I don't see why we need a custom context provider. I would prefer we try to have a `useEffect` inside of `useGridData` to detect only the first time `runId` is specified in the url params.
>
> It's been some time now so i don't remember the exact details, i think i tried that as first version but it didn't work for some reason. I wouldn't add context unnecessarily, as a I'm normally very anti global variables. :D
>
> The context is needed to transfer the first selection between `useSelection` and `useGridData`. There are 14 different usages of these and it's today not 100% clear how they are connected, because the data is transferred between them using the already global variable `useQuery` and `setSearchParams` :) The alternative would have been to use the return value of `useSelection` and prop-drill it into every use of `useGridData` instead.
>
> The use of query params also introduces an ordering problem. If `useGridData` is created after `useSelection`. Then there is a risk that `setSearchParams` inside `useSelection` is called before any `useGridData` tries to read `searchParams` to lock the runId. I think this was actually the main problem that prevented me from storing the first selection inside `useGridData`, but i can't remember for sure.
>
> (The placement of `firstRunIdSetByUrl = useContext(DagRunSelectionContext)` inside `useSelection` is purely cosmetic, to centralize all selection logic in one place. That is possible to easily move into `useGridData`)
>
> If you see any better way to do this, i would be more than happy to use some other approach instead. Also feel free to overwrite this PR if you have some other example for this already.
That makes sense. You are right the ordering is a bit tricky since selection/griddata also edit the url params.
Let me pull this down locally and poke around today.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "sinwoobang (via GitHub)" <gi...@apache.org>.
sinwoobang commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1802300696
This bug significantly impacts the efficiency of navigating DagRuns, particularly when dealing with a high volume of items. The ability to quickly identify a specific DagRun is critical to my workflow, and the current issue hinders this process. A prompt resolution would greatly enhance productivity and is eagerly anticipated.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on code in PR #34887:
URL: https://github.com/apache/airflow/pull/34887#discussion_r1392647818
##########
airflow/www/views.py:
##########
@@ -3467,25 +3468,44 @@ def grid_data(self):
if num_runs is None:
num_runs = conf.getint("webserver", "default_dag_run_display_number")
- try:
- base_date = timezone.parse(request.args["base_date"], strict=True)
- except (KeyError, ValueError):
- base_date = dag.get_latest_execution_date() or timezone.utcnow()
+ dagrun = None
+ if run_id:
+ with create_session() as session:
+ dagrun = dag.get_dagrun(run_id=run_id, session=session)
+ if not dagrun:
+ return {"error": f"can't find dag_run_id={run_id}"}, 404
+ base_date = dagrun.execution_date
+ else:
+ try:
+ base_date = timezone.parse(request.args["base_date"], strict=True)
+ except (KeyError, ValueError):
+ base_date = dag.get_latest_execution_date() or timezone.utcnow()
with create_session() as session:
query = select(DagRun).where(DagRun.dag_id == dag.dag_id, DagRun.execution_date <= base_date)
run_type = request.args.get("run_type")
if run_type:
+ if run_id:
+ return {"error": "Can not provide filters when dag_run_id filter is selected."}, 400
query = query.where(DagRun.run_type == run_type)
run_state = request.args.get("run_state")
if run_state:
+ if run_id:
+ return {"error": "Can not provide filters when dag_run_id filter is selected."}, 400
query = query.where(DagRun.state == run_state)
dag_runs = wwwutils.sorted_dag_runs(
query, ordering=dag.timetable.run_ordering, limit=num_runs, session=session
)
+ if dagrun:
+ found_requested_run_id = any(True for d in dag_runs if d.run_id == run_id)
Review Comment:
This query is limited to `num_runs` so it shouldn't be an issue. This is the final response sent to UI so query must be executed anyway and if it contains 10k runs it would have other bigger issues. The check added here is more of final assert.
The query that find the base_date based on provided run_id is run further up on line 3474. Here it uses DB-queries instead of python.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #34887:
URL: https://github.com/apache/airflow/pull/34887#discussion_r1496547257
##########
airflow/www/views.py:
##########
@@ -3465,25 +3466,42 @@ def grid_data(self):
if num_runs is None:
num_runs = conf.getint("webserver", "default_dag_run_display_number")
- try:
- base_date = timezone.parse(request.args["base_date"], strict=True)
- except (KeyError, ValueError):
- base_date = dag.get_latest_execution_date() or timezone.utcnow()
+ dagrun = None
+ if run_id:
+ with create_session() as session:
+ dagrun = dag.get_dagrun(run_id=run_id, session=session)
+ if not dagrun:
+ return {"error": f"can't find dag_run_id={run_id}"}, 404
+ base_date = dagrun.execution_date
Review Comment:
I think this was specific to the widget so we're ok here.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1831229595
Is it possible to take the base date changes separately as an improvement? This bug makes the most recent versions of airflow completely unusable for us, would be great to have some kind of fix in asap.
The situation where one navigates from old run to new run isn't even possible to reach today due to this bug so i don't think we are regressing anything? Maybe i'm missing something, can you explain the scenario a bit more in detail? Does this change break the base_date selector in any other way?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1830785350
1. Do you mind rebasing?
2. Can we change the base date in the UI as well? Otherwise there is no way in the UI to reset the selection and get to the most recent dag runs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1856537227
@hterik Ok I'm happy to clean up this filters+main logic. Mind to rebase the PR first?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Base date for fetching dag grid view must include selected run_id [airflow]
Posted by "hterik (via GitHub)" <gi...@apache.org>.
hterik commented on PR #34887:
URL: https://github.com/apache/airflow/pull/34887#issuecomment-1818390814
> I don't see why we need a custom context provider. I would prefer we try to have a `useEffect` inside of `useGridData` to detect only the first time `runId` is specified in the url params.
It's been some time now so i don't remember the exact details, i think i tried that as first version but it didn't work for some reason. I wouldn't add context unnecessarily, as a I'm normally very anti global variables. :D
The context is needed to transfer the first selection between `useSelection` and `useGridData`. There are 14 different usages of these and it's today not 100% clear how they are connected, because the data is transferred between them using the already global variable `useQuery` and `setSearchParams` :)
The alternative would have been to use the return value of `useSelection` and prop-drill it into every use of `useGridData` instead.
The use of query params also introduces an ordering problem. If `useGridData` is created after `useSelection`. Then there is a risk that `setSearchParams` inside `useSelection` is called before any `useGridData` tries to read `searchParams` to lock the runId. I think this was actually the main problem that prevented me from storing the first selection inside `useGridData`, but i can't remember for sure.
---------------
(The placement of `firstRunIdSetByUrl = useContext(DagRunSelectionContext)` inside `useSelection` is purely cosmetic, to centralize all selection logic in one place. That is possible to easily move into `useGridData`)
---------------
If you see any better way to do this, i would be more than happy to use some other approach instead. Also feel free to overwrite this PR if you have some other example for this already.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org