-
Notifications
You must be signed in to change notification settings - Fork 392
feat(dashboard): enhance dashboard UI and fix Ray runner state reporting #6008
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,12 +96,25 @@ def run_iter( | |
| output_schema = builder.schema() | ||
|
|
||
| # Optimize the logical plan. | ||
| ctx._notify_query_start(query_id, PyQueryMetadata(output_schema._schema, builder.repr_json())) | ||
| import sys | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Import statements should be at the top of the file. Move Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: daft/runners/native_runner.py
Line: 99:99
Comment:
[P2] Import statements should be at the top of the file. Move `import sys` to the imports section at the beginning of the file (around line 3-4) per the project's import style guidelines.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree |
||
|
|
||
| entrypoint = "python " + " ".join(sys.argv) | ||
| ctx._notify_query_start( | ||
| query_id, | ||
| PyQueryMetadata( | ||
| output_schema._schema, | ||
| builder.repr_json(), | ||
| "Native (Swordfish)", | ||
| ray_dashboard_url=None, | ||
| entrypoint=entrypoint, | ||
| ), | ||
| ) | ||
| ctx._notify_optimization_start(query_id) | ||
| builder = builder.optimize(ctx.daft_execution_config) | ||
| ctx._notify_optimization_end(query_id, builder.repr_json()) | ||
|
|
||
| plan = LocalPhysicalPlan.from_logical_plan_builder(builder._builder) | ||
|
|
||
| executor = NativeExecutor() | ||
| results_gen = executor.run( | ||
| plan, | ||
|
|
@@ -112,8 +125,10 @@ def run_iter( | |
| ) | ||
|
|
||
| try: | ||
| total_rows = 0 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why collect this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a debug code; I deleted something here. |
||
| for result in results_gen: | ||
| ctx._notify_result_out(query_id, result.partition()) | ||
| total_rows += len(result.partition()) | ||
| yield result | ||
| except KeyboardInterrupt as e: | ||
| query_result = PyQueryResult(QueryEndState.Canceled, "Query canceled by the user.") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import logging | ||
| import os | ||
| import sys | ||
| import time | ||
| import uuid | ||
| from collections.abc import Generator, Iterable, Iterator | ||
|
|
@@ -45,6 +48,9 @@ | |
| FileFormatConfig, | ||
| FileInfos, | ||
| IOConfig, | ||
| PyQueryMetadata, | ||
| PyQueryResult, | ||
| QueryEndState, | ||
| ) | ||
| from daft.datatype import DataType | ||
| from daft.filesystem import glob_path_with_stats | ||
|
|
@@ -548,19 +554,84 @@ def run_iter( | |
| ctx = get_context() | ||
| query_id = str(uuid.uuid4()) | ||
| daft_execution_config = ctx.daft_execution_config | ||
| output_schema = builder.schema() | ||
|
|
||
| # Optimize the logical plan. | ||
| builder = builder.optimize(daft_execution_config) | ||
|
|
||
| distributed_plan = DistributedPhysicalPlan.from_logical_plan_builder( | ||
| builder._builder, query_id, daft_execution_config | ||
| # Notify query start | ||
| ray_dashboard_url = None | ||
| try: | ||
| ray_dashboard_url = ray.worker.get_dashboard_url() | ||
| if ray_dashboard_url: | ||
| if not ray_dashboard_url.startswith("http"): | ||
| ray_dashboard_url = f"http://{ray_dashboard_url}" | ||
|
|
||
| # Try to append Job ID | ||
| try: | ||
| job_id = ray.get_runtime_context().get_job_id() | ||
| if job_id: | ||
| ray_dashboard_url = f"{ray_dashboard_url}/#/jobs/{job_id}" | ||
| except Exception: | ||
| pass | ||
| except Exception: | ||
| pass | ||
|
|
||
| entrypoint = "python " + " ".join(sys.argv) | ||
|
|
||
| ctx._notify_query_start( | ||
| query_id, | ||
| PyQueryMetadata( | ||
| output_schema._schema, builder.repr_json(), "Ray (Flotilla)", ray_dashboard_url, entrypoint | ||
| ), | ||
| ) | ||
| if self.flotilla_plan_runner is None: | ||
| self.flotilla_plan_runner = FlotillaRunner() | ||
| ctx._notify_optimization_start(query_id) | ||
|
|
||
| yield from self.flotilla_plan_runner.stream_plan( | ||
| distributed_plan, self._part_set_cache.get_all_partition_sets() | ||
| ) | ||
| # Log Dashboard URL if configured | ||
| dashboard_url = os.environ.get("DAFT_DASHBOARD_URL") | ||
| if dashboard_url: | ||
| print(f"Daft Dashboard: {dashboard_url}/query/{query_id}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove print
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, I changed print to logger, mainly to clearly show users how to access the dashboard. What do you think? |
||
|
|
||
| try: | ||
| # Optimize the logical plan. | ||
| builder = builder.optimize(daft_execution_config) | ||
| ctx._notify_optimization_end(query_id, builder.repr_json()) | ||
|
|
||
| distributed_plan = DistributedPhysicalPlan.from_logical_plan_builder( | ||
| builder._builder, query_id, daft_execution_config | ||
| ) | ||
| physical_plan_json = distributed_plan.repr_json() | ||
| ctx._notify_exec_start(query_id, physical_plan_json) | ||
|
|
||
| if self.flotilla_plan_runner is None: | ||
| self.flotilla_plan_runner = FlotillaRunner() | ||
|
|
||
| total_rows = 0 | ||
| for result in self.flotilla_plan_runner.stream_plan( | ||
| distributed_plan, self._part_set_cache.get_all_partition_sets() | ||
| ): | ||
| if result.metadata() is not None: | ||
| total_rows += result.metadata().num_rows | ||
| yield result | ||
|
|
||
| # Mark all operators as finished to clean up the Dashboard UI before notify_exec_end | ||
| try: | ||
| plan_dict = json.loads(physical_plan_json) | ||
|
|
||
| def notify_end(node: dict[str, Any]) -> None: | ||
| if "children" in node: | ||
| for child in node["children"]: | ||
| notify_end(child) | ||
| if "id" in node: | ||
| ctx._notify_exec_operator_end(query_id, node["id"]) | ||
|
|
||
| notify_end(plan_dict) | ||
| except Exception: | ||
| pass | ||
|
|
||
| ctx._notify_exec_end(query_id) | ||
| ctx._notify_query_end(query_id, PyQueryResult(QueryEndState.Finished, "")) | ||
|
|
||
| except Exception as e: | ||
| ctx._notify_query_end(query_id, PyQueryResult(QueryEndState.Failed, str(e))) | ||
| raise | ||
|
|
||
| def run_iter_tables( | ||
| self, builder: LogicalPlanBuilder, results_buffer_size: int | None = None | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why emit the ray dashboard URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah makes sense, good idea