Skip to content

core: harden ImpressManager lifecycle, add GPU discovery, filterable logging - #60

Open
drawadiagram wants to merge 1 commit into
mainfrom
update/core-manager-0926
Open

core: harden ImpressManager lifecycle, add GPU discovery, filterable logging#60
drawadiagram wants to merge 1 commit into
mainfrom
update/core-manager-0926

Conversation

@drawadiagram

Copy link
Copy Markdown
Contributor

Splits the impress core framework changes out of #59 so they can be reviewed on their own. First of four; the other three depend on this one (both workflows import find_gpus from impress, and both set IMPRESS_SESSION_DIR).

ImpressManager

  • Session dirs move off scratch. asyncflow's session dir is now created under IMPRESS_SESSION_DIR (default: the system temp dir) rather than cwd, which exhausted the scratch inode quota over many HPC runs.
  • Engine shutdown moved into a finally. A pipeline raising mid-run used to leak the WorkflowEngine and its backend. This is also where the per-example await manager.flow.shutdown() went — answering @AymenFJA's question on Impress fixes #59 about its removal from run_protein_binding.py; the examples no longer have to remember it.
  • submit_new_pipelines() before start() now raises a clear RuntimeError instead of an AttributeError on a missing self.flow.
  • Failed pipelines are reported as failures. Each pipeline's future is carried through cleanup, so a pipeline that raised is logged via the new logger.pipeline_failed() rather than silently logged as completed.
  • activity_summary() stops reporting 0. The buffered count is captured before the buffer is cleared.

ImpressLogger

  • min_level filtering on every level method and on pipeline_log(); defaults to DEBUG, so nothing is filtered unless opted in.
  • New pipeline_failed(name, exc).
  • ⚠️ error/critical now go to output_stream instead of stderr. Deliberate — under Dragon the job's stdout is the log anyone actually reads, and errors were being split into a stream nobody checks. Flagging it because it is a behaviour change for anything parsing stderr.

Other

  • New impress.gpu.find_gpus(), re-exported from the package root: CUDA_VISIBLE_DEVICES, else nvidia-smi, else [].
  • ImpressBasePipeline.finalize() is no longer abstract — pipelines needing no cleanup shouldn't have to define an empty override.
  • Callable imported from collections.abc.
  • radical-asyncflow>=0.4.0, the only pyproject.toml change. WorkflowEngine.create(work_dir=...) does not exist in 0.3.1 or earlier, so the session-dir change above needs a floor. Verified by unpacking both sdists.

Python version support

Python 3.9/3.10 support is retained. #59 dropped py39/py310 from the CI matrix; @AymenFJA objected and @mgoliyad agreed to put them back, so that change is reverted here and this PR does not touch .github/workflows/tests.yml or tox.ini at all.

Keeping 3.9 means keeping except asyncio.TimeoutError in test_pipeline_management.py rather than the shorter except TimeoutError: asyncio.TimeoutError only became an alias of the builtin in 3.11, so on 3.9/3.10 the short spelling silently fails to catch the timeout. Nothing else in src/ needs 3.10+ — the generics are PEP 585, which 3.9 evaluates fine — and radical-asyncflow itself declares requires-python >=3.9.

Tests

Adds test_logger.py, test_pipeline_base.py, test_pipeline_setup.py, plus regression tests for the two fixes above: test_start_exception_still_shuts_down_engine and test_submit_before_start_raises. 94 unit tests pass; ruff check and ruff format --check clean.

Checked against the existing examples

examples/discontinuous_scaffolds/, dummy.py and dummy_adaptive.py aren't touched by this PR, so I verified it doesn't break them. The one real risk was double shutdown — 8 entry points on main call await manager.flow.shutdown() after start(), which now runs twice. Exercised end-to-end with a real ImpressManager + LocalExecutionBackend: the second call returns cleanly and the process exits 0. Not exercisable for rhapsody's DragonExecutionBackend locally; PRs 2 and 3 delete those now-redundant calls anyway.

Unrelated pre-existing breakage noticed while checking: examples/dummy.py and dummy_adaptive.py both from radical.asyncflow import ConcurrentExecutionBackend, which moved to rhapsody.backends — they fail at import on main today. Not touched here; worth its own issue.

Replaces part of #59.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lo8DwSbyvdWZRkkkka6gA2

Core framework changes extracted from the impress_fixes branch.

ImpressManager:
- Write asyncflow session dirs to IMPRESS_SESSION_DIR (default: tempdir)
  instead of cwd, which exhausted scratch inodes over many HPC runs.
- Shut the WorkflowEngine down in a finally block so a pipeline raising
  mid-run no longer leaks the engine and its backend. This is where the
  per-example `await manager.flow.shutdown()` call went; the examples no
  longer have to remember it.
- Guard submit_new_pipelines() with a clear RuntimeError when called
  before start(), instead of failing on a missing self.flow attribute.
- Carry each pipeline's future through cleanup so a failed pipeline is
  reported via logger.pipeline_failed() rather than logged as completed.
- Capture the buffered-pipeline count before clearing the buffer, so
  activity_summary() stops always reporting 0 new submissions.

ImpressLogger:
- Add min_level filtering to every level method and pipeline_log().
- Add pipeline_failed(name, exc).
- Route error/critical to output_stream rather than stderr: under Dragon
  the job's stdout is the reviewable log, and errors were being split
  into a separate stream nobody reads.

Other:
- New impress.gpu.find_gpus(), re-exported from the package root:
  CUDA_VISIBLE_DEVICES, else nvidia-smi, else an empty list.
- ImpressBasePipeline.finalize() is no longer abstract; pipelines that
  need no cleanup should not have to define an empty override.
- Import Callable from collections.abc (deprecated in typing).
- Pin radical-asyncflow>=0.4.0: WorkflowEngine.create(work_dir=...) does
  not exist in 0.3.1 and earlier, so the session-dir change above needs
  a floor on the dependency.

Python 3.9/3.10 support is retained, per review of PR #59. That means
keeping `except asyncio.TimeoutError` in test_pipeline_management.py --
asyncio.TimeoutError only became an alias of the builtin TimeoutError in
3.11, so the shorter spelling silently fails to catch the timeout on 3.9
and 3.10. Nothing else in src/ needs 3.10+: the generics here are PEP
585, which 3.9 evaluates fine, and radical-asyncflow itself declares
requires-python >=3.9.

Adds unit coverage for the logger, the pipeline base class, and
PipelineSetup, plus regression tests for the shutdown and
submit-before-start fixes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lo8DwSbyvdWZRkkkka6gA2
Comment thread src/impress/gpu.py
import subprocess


def find_gpus() -> list[int]:

Copy link
Copy Markdown
Collaborator

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_gpus being 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_gpus as 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.

# 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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason behind this architectural change of making WorkflowEngine an internal component?

@AymenFJA

Copy link
Copy Markdown
Collaborator

@drawadiagram I added 2 comments above. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants