feat: add unified context-aware logging system#2
Open
devin-ai-integration[bot] wants to merge 2 commits intomasterfrom
Open
feat: add unified context-aware logging system#2devin-ai-integration[bot] wants to merge 2 commits intomasterfrom
devin-ai-integration[bot] wants to merge 2 commits intomasterfrom
Conversation
New src/crab/log/ package with: - CrabLogger: hierarchical context nesting (worker > experiment > run > app) - RichFormatter: ANSI-colored tree-style output - PlainFormatter: grep-friendly bracketed output - StreamHandler: stdout (-> slurm_output.log under SLURM) - TUIHandler: routes records to Textual RichLog widget - Live subprocess output streaming via background threads - Thread-safe concurrent logging with write locks - CRAB_LOG_LEVEL env var + --log-level CLI flag Integration: - engine.py: CrabLogger replaces log_callback, context nesting per experiment/run/app - orchestrator.py: all print() replaced, --log-level flag added - slurm.py/mpi.py: debug prints removed (wl_managers stay logger-agnostic) - controller.py: TUIHandler wired to RichLog widget - wrappers: stray prints removed from microbench_common, miniFE, ib_send_lat Co-Authored-By: Matteo Marcelletti <marcellettimatteo02@gmail.com>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Matteo Marcelletti <marcellettimatteo02@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Replaces all inconsistent logging approaches (13+ raw
print()calls, manual[DEBUG]/[INFO]tags,sys.stderrwrites, and the partiallog_callback: Callableabstraction) with a unified, context-aware, hierarchical logging system.New package
src/crab/log/:logger.py—CrabLoggerwith hierarchical context nesting viaenter(), thread-safe emit with a shared lock, live subprocess stdout streaming via background threadsformatters.py—RichFormatter(ANSI-colored tree-style) andPlainFormatter(grep-friendly brackets)handlers.py—StreamHandler(stdout → slurm_output.log under SLURM),TUIHandler(Rich markup → Textual RichLog widget)__init__.py—get_logger()factory, readsCRAB_LOG_LEVELenv varIntegration changes:
engine.py—EngineandExperimentRunneracceptCrabLoggerinstead ofCallable.run_job()andend_job()accept a logger. Live streaming enabled for concurrent apps viastream_process()background threads.orchestrator.py— Allprint()replaced. New--log-levelCLI flag added. Workers readCRAB_LOG_LEVELfrom environment.controller.py—TUIControllerbuilds aCrabLoggerwith aTUIHandlerwired to the RichLog widget callback, replacing raw Rich markup strings in log calls.slurm.py/mpi.py— Debug prints removed; wl_managers stay logger-agnostic.microbench_common.py,miniFE.py,ib_send_lat.py) — Strayprint()calls removed.Review & Testing Checklist for Human
run_job(),end_job(),wait_timed(),Engine(), andExperimentRunner()all have changed signatures. Verify no call site was missed (search for old patterns likelog_callback=,log_fn=,run_job(withoutlogger=). A missed call site will cause a runtime crash on the cluster._stream_threadattribute on job objects:job._stream_threadis set dynamically (not declared on the job/app class). Verify this doesn't conflict with existing attributes and that allhasattrguards are sufficient. Consider whether this should be a proper attribute on the base class.concurrent = len(static_schedule) > 1 or len(dependency_map) > 0determines whether to live-stream stdout. Verify this correctly identifies concurrent vs sequential scenarios for your experiment configurations. A false positive means unnecessary threads; a false negative means no live output for concurrent apps.TUIControllernow has both aStreamHandler(fromget_logger()) AND aTUIHandler. This means TUI mode logs to both stdout and the widget. Verify this is acceptable or if the StreamHandler should be omitted in TUI mode.slurm_output.logcontains properly formatted, colored output, (b)tail -f slurm_output.logrenders colors correctly, (c) concurrent app output is interleaved correctly with context labels, (d)--log-level DEBUGshows debug output, and (e) the TUI log tab displays records correctly.Notes
traceback.print_exc()calls in fatal error handlers still write directly to stderr — this is intentional since the logger itself may be in a broken state at that point.workerpool_scheduler.pylogging was intentionally left untouched per requirements (it's a separate sub-benchmark with its ownscheduler.log).BenchmarkStateimport was removed fromcontroller.py— verify it was truly unused before merging.Link to Devin session: https://app.devin.ai/sessions/fc7b895897f54186a4d154c93d00b658
Requested by: @SharkGamerZ