-
Notifications
You must be signed in to change notification settings - Fork 1
core: harden ImpressManager lifecycle, add GPU discovery, filterable logging #60
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
Open
drawadiagram
wants to merge
1
commit into
main
Choose a base branch
from
update/core-manager-0926
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+843
−122
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # SLURM log output (all workflows) | ||
| */logs/ | ||
|
|
||
| # Per-pipeline task directories written to cwd | ||
| */p*/ | ||
|
|
||
| # Dragon telemetry session dirs | ||
| */telemetry/ | ||
|
|
||
| # Run metadata written to cwd | ||
| */runinfo | ||
|
|
||
| # Legacy output dirs | ||
| */myoutputs/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import os | ||
| import subprocess | ||
|
|
||
|
|
||
| def find_gpus() -> list[int]: | ||
| """Return GPU IDs available to this process. | ||
|
|
||
| Checks CUDA_VISIBLE_DEVICES first, then nvidia-smi. | ||
| Falls back to an empty list when neither yields results. | ||
| """ | ||
| val = os.environ.get("CUDA_VISIBLE_DEVICES", "") | ||
| ids = [int(g) for g in val.split(",") if g.strip().isdigit()] | ||
| if ids: | ||
| return ids | ||
|
|
||
| try: | ||
| out = subprocess.run( | ||
| ["nvidia-smi", "--query-gpu=index", "--format=csv,noheader"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=5, | ||
| ) | ||
| if out.returncode == 0: | ||
| return [ | ||
| int(ln.strip()) | ||
| for ln in out.stdout.splitlines() | ||
| if ln.strip().isdigit() | ||
| ] | ||
| except Exception: | ||
| pass | ||
|
|
||
| return [] | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| import asyncio | ||
| from collections.abc import Awaitable | ||
| from typing import Any, Callable, Optional, Union | ||
| import os | ||
| import tempfile | ||
| from collections.abc import Awaitable, Callable | ||
| from typing import Any, Optional, Union | ||
|
|
||
| from radical.asyncflow import WorkflowEngine | ||
|
|
||
|
|
@@ -43,6 +45,7 @@ def __init__( | |
| self._telemetry_config: dict[str, Any] = telemetry_config or {} | ||
| self._telemetry_subscribers: list[Callable] = telemetry_subscribers or [] | ||
| self.telemetry: Any = None | ||
| self.flow: Optional[WorkflowEngine] = None | ||
|
|
||
| def _normalize_pipeline_setup( | ||
| self, setup: Union[dict[str, Any], PipelineSetup] | ||
|
|
@@ -77,6 +80,10 @@ def submit_new_pipelines( | |
| ValueError: If pipeline type is not a subclass | ||
| of ImpressBasePipeline | ||
| """ | ||
| if self.flow is None: | ||
| raise RuntimeError( | ||
| "ImpressManager.start() must be called before submit_new_pipelines()" | ||
| ) | ||
| for setup_input in pipeline_setups: | ||
| # Normalize to PipelineSetup object | ||
| setup = self._normalize_pipeline_setup(setup_input) | ||
|
|
@@ -136,113 +143,139 @@ async def start( | |
| """ | ||
| self.logger.separator("IMPRESS MANAGER STARTING") | ||
|
|
||
| self.flow: WorkflowEngine = await WorkflowEngine.create( | ||
| backend=self.execution_backend | ||
| # Write asyncflow session dirs to /tmp (node-local, no quota) instead of | ||
| # cwd on scratch, which exhausts inodes over many runs. | ||
| _session_base = os.environ.get("IMPRESS_SESSION_DIR", tempfile.gettempdir()) | ||
| self.flow = await WorkflowEngine.create( | ||
|
Collaborator
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. What is the reason behind this architectural change of making |
||
| backend=self.execution_backend, | ||
| work_dir=_session_base, | ||
| ) | ||
|
|
||
| if self._telemetry_config: | ||
| self.telemetry = await self.flow.start_telemetry(**self._telemetry_config) | ||
| for fn in self._telemetry_subscribers: | ||
| self.telemetry.subscribe(fn) | ||
|
|
||
| self.logger.manager_starting(len(pipeline_setups)) | ||
|
|
||
| self.submit_new_pipelines(pipeline_setups) | ||
|
|
||
| while True: | ||
| any_activity: bool = False | ||
| completed_pipelines: list[ImpressBasePipeline] = [] | ||
|
|
||
| for pipeline, pipeline_future in list(self.pipeline_tasks.items()): | ||
| # Check if pipeline needs adaptive step and isn't already running one | ||
| if ( | ||
| getattr(pipeline, "invoke_adaptive_step", False) | ||
| and pipeline not in self.adaptive_tasks | ||
| ): | ||
| adaptive_task: asyncio.Task = asyncio.create_task( | ||
| self._run_adaptive_fn(pipeline) | ||
| try: | ||
| if self._telemetry_config: | ||
| self.telemetry = await self.flow.start_telemetry( | ||
| **self._telemetry_config | ||
| ) | ||
| for fn in self._telemetry_subscribers: | ||
| self.telemetry.subscribe(fn) | ||
|
|
||
| self.logger.manager_starting(len(pipeline_setups)) | ||
|
|
||
| self.submit_new_pipelines(pipeline_setups) | ||
|
|
||
| while True: | ||
| any_activity: bool = False | ||
| completed_pipelines: list[tuple] = [] | ||
|
|
||
| for pipeline, pipeline_future in list(self.pipeline_tasks.items()): | ||
| # Check if pipeline needs adaptive step and isn't running one yet | ||
| if ( | ||
| getattr(pipeline, "invoke_adaptive_step", False) | ||
| and pipeline not in self.adaptive_tasks | ||
| ): | ||
| adaptive_task: asyncio.Task = asyncio.create_task( | ||
| self._run_adaptive_fn(pipeline) | ||
| ) | ||
| self.adaptive_tasks[pipeline] = adaptive_task | ||
| any_activity = True | ||
|
|
||
| # Check if pipeline has new config ready | ||
| config: Optional[dict[str, Any]] = ( | ||
| pipeline.get_child_pipeline_request() | ||
| ) | ||
| self.adaptive_tasks[pipeline] = adaptive_task | ||
| any_activity = True | ||
|
|
||
| # Check if pipeline has new config ready | ||
| config: Optional[dict[str, Any]] = pipeline.get_child_pipeline_request() | ||
|
|
||
| if config: | ||
| self.logger.child_pipeline_submitted(config["name"], pipeline.name) | ||
| # Convert dict to PipelineSetup for consistency | ||
| child_setup = PipelineSetup.from_dict(config) | ||
| self.new_pipeline_buffer.append(child_setup) | ||
| any_activity = True | ||
| if config: | ||
| self.logger.child_pipeline_submitted( | ||
| config["name"], pipeline.name | ||
| ) | ||
| # Convert dict to PipelineSetup for consistency | ||
| child_setup = PipelineSetup.from_dict(config) | ||
| self.new_pipeline_buffer.append(child_setup) | ||
| any_activity = True | ||
|
|
||
| # Check if parent should be killed | ||
| if getattr(pipeline, "kill_parent", False): | ||
| self.logger.pipeline_killed(pipeline.name) | ||
| pipeline_future.cancel() | ||
| completed_pipelines.append((pipeline, pipeline_future)) | ||
| continue | ||
|
|
||
| # Check if parent should be killed | ||
| if getattr(pipeline, "kill_parent", False): | ||
| self.logger.pipeline_killed(pipeline.name) | ||
| pipeline_future.cancel() | ||
| completed_pipelines.append(pipeline) | ||
| continue | ||
|
|
||
| # Check if pipeline is done - but only mark as completed | ||
| # if adaptive task is also done | ||
| if pipeline_future.done(): | ||
| # If there's an adaptive task running, don't mark as completed yet | ||
| # Check if pipeline is done - but only mark as completed | ||
| # if adaptive task is also done | ||
| if pipeline_future.done(): | ||
| # Adaptive task still running — wait before marking completed | ||
| if pipeline in self.adaptive_tasks: | ||
| adaptive_task = self.adaptive_tasks[pipeline] | ||
| if not adaptive_task.done(): | ||
| continue | ||
|
|
||
| completed_pipelines.append((pipeline, pipeline_future)) | ||
|
|
||
| # Clean up completed pipelines - but only if their | ||
| # adaptive tasks are also done | ||
| actually_completed: list[ImpressBasePipeline] = [] | ||
| for pipeline, future in completed_pipelines: | ||
| # Double-check: only clean up if adaptive task is | ||
| # done or doesn't exist | ||
| if pipeline in self.adaptive_tasks: | ||
| adaptive_task = self.adaptive_tasks[pipeline] | ||
| if not adaptive_task.done(): | ||
| continue | ||
| self.adaptive_tasks.pop(pipeline) | ||
|
|
||
| self.pipeline_tasks.pop(pipeline, None) | ||
| exc = None | ||
| if future.done() and not future.cancelled(): | ||
| try: | ||
| exc = future.exception() | ||
| except Exception: | ||
| pass | ||
| if exc is not None: | ||
| self.logger.pipeline_failed(pipeline.name, exc) | ||
| else: | ||
| self.logger.pipeline_completed(pipeline.name) | ||
| actually_completed.append(pipeline) | ||
|
|
||
| completed_pipelines = actually_completed | ||
|
|
||
| # Clean up completed adaptive tasks | ||
| completed_adaptive: list[ImpressBasePipeline] = [] | ||
| for pipeline, adaptive_task in list(self.adaptive_tasks.items()): | ||
| if adaptive_task.done(): | ||
| completed_adaptive.append(pipeline) | ||
|
|
||
| for pipeline in completed_adaptive: | ||
| self.adaptive_tasks.pop(pipeline, None) | ||
|
|
||
| # Submit new pipelines; capture count before clearing so | ||
| # activity_summary reports the real number submitted. | ||
| if self.new_pipeline_buffer: | ||
| buffered_count = len(self.new_pipeline_buffer) | ||
| self.submit_new_pipelines(self.new_pipeline_buffer) | ||
| self.new_pipeline_buffer.clear() | ||
| any_activity = True | ||
| else: | ||
| buffered_count = 0 | ||
|
|
||
| # Log activity summary periodically | ||
| if any_activity: | ||
| self.logger.activity_summary( | ||
| len(self.pipeline_tasks), | ||
| len(self.adaptive_tasks), | ||
| buffered_count, | ||
| ) | ||
|
|
||
| completed_pipelines.append(pipeline) | ||
|
|
||
| # Clean up completed pipelines - but only if their | ||
| # adaptive tasks are also done | ||
| actually_completed: list[ImpressBasePipeline] = [] | ||
| for pipeline in completed_pipelines: | ||
| # Double-check: only clean up if adaptive task is | ||
| # done or doesn't exist | ||
| if pipeline in self.adaptive_tasks: | ||
| adaptive_task = self.adaptive_tasks[pipeline] | ||
| if not adaptive_task.done(): | ||
| continue | ||
| self.adaptive_tasks.pop(pipeline) | ||
|
|
||
| self.pipeline_tasks.pop(pipeline, None) | ||
| self.logger.pipeline_completed(pipeline.name) | ||
| actually_completed.append(pipeline) | ||
|
|
||
| completed_pipelines = actually_completed | ||
|
|
||
| # Clean up completed adaptive tasks | ||
| completed_adaptive: list[ImpressBasePipeline] = [] | ||
| for pipeline, adaptive_task in list(self.adaptive_tasks.items()): | ||
| if adaptive_task.done(): | ||
| completed_adaptive.append(pipeline) | ||
|
|
||
| for pipeline in completed_adaptive: | ||
| self.adaptive_tasks.pop(pipeline, None) | ||
|
|
||
| # Submit new pipelines | ||
| if self.new_pipeline_buffer: | ||
| self.submit_new_pipelines(self.new_pipeline_buffer) | ||
| self.new_pipeline_buffer.clear() | ||
| any_activity = True | ||
|
|
||
| # Log activity summary periodically | ||
| if any_activity: | ||
| self.logger.activity_summary( | ||
| len(self.pipeline_tasks), | ||
| len(self.adaptive_tasks), | ||
| len(self.new_pipeline_buffer), | ||
| ) | ||
| # Exit condition | ||
| if ( | ||
| not self.pipeline_tasks | ||
| and not self.new_pipeline_buffer | ||
| and not self.adaptive_tasks | ||
| ): | ||
| self.logger.manager_exiting() | ||
| self.logger.separator("IMPRESS MANAGER FINISHED") | ||
| break | ||
|
|
||
| # Exit condition | ||
| if ( | ||
| not self.pipeline_tasks | ||
| and not self.new_pipeline_buffer | ||
| and not self.adaptive_tasks | ||
| ): | ||
| self.logger.manager_exiting() | ||
| self.logger.separator("IMPRESS MANAGER FINISHED") | ||
| break | ||
|
|
||
| if not any_activity: | ||
| await asyncio.sleep(0.5) | ||
| if not any_activity: | ||
| await asyncio.sleep(0.5) | ||
| finally: | ||
| await self.flow.shutdown() | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What about a multi-node setup? How is
find_gpusbeing spawned?GPU devices should be detected by the backend and then made visible to the application (the IMPRESS pipeline) through backend tasks.
We can keep
find_gpusas a helper utility, but in general, having IMPRESS discover GPUs itself is a layer violation. IMPRESS should not need to know how or where GPUs are discovered; that responsibility belongs to the backend/infrastructure layer i.e:asyncflow-->rhapsody.