Skip to content

Supporting RooUtil in Dask#361

Merged
AndrewEdmonds11 merged 38 commits intoMu2e:mainfrom
AndrewEdmonds11:roodask2
Apr 22, 2026
Merged

Supporting RooUtil in Dask#361
AndrewEdmonds11 merged 38 commits intoMu2e:mainfrom
AndrewEdmonds11:roodask2

Conversation

@AndrewEdmonds11
Copy link
Copy Markdown
Collaborator

This PR adds a roodask.py script for running RooUtil macros using dask. This is mostly vibe-coded with Claude Opus 4.6. There is a README (generated by the AI).

@AndrewEdmonds11
Copy link
Copy Markdown
Collaborator Author

Prompt: "Please review this PR for critical bugs"
Model: Claude Sonnet 4.6
Output:

Here's a review of critical bugs found in rooutil/roodask/roodask.py:


🐛 Critical Bugs

1. TypeError in hadd_target path resolution

File: roodask.py, in the hadd section of main()

hadd_target = Path(args.hadd)
if not hadd_target.is_absolute():
    hadd_target = output_dir / hadd_target  # ❌ BUG

Problem: output_dir is a str (set via str(Path(...).resolve()) earlier), not a Path. The / operator only works when the left-hand side is a Path. This will raise a TypeError: unsupported operand type(s) for /: 'str' and 'PosixPath' at runtime whenever --hadd is given a relative path.

Fix:

hadd_target = Path(output_dir) / hadd_target

2. work_dir inconsistency between submitter and workers

File: roodask.py, in client.submit(...) call

work_dir=args.work_dir,   # ❌ passes the raw/relative CLI string

Problem: args.work_dir is passed as-is (e.g. "./work") to the workers, but the resolved work_dir (a Path) is what the submitter uses locally. On a shared filesystem this is fine if workers share the CWD, but if workers resolve paths differently or the scheduler runs from a different CWD, per-job filelist files will be written to the wrong location (or fail to be found by the binary). The resolved absolute path should be passed instead.

Fix:

work_dir=str(work_dir),   # work_dir is already resolved to an absolute Path

3. --skip-compile / --force-compile mutual exclusion not enforced

File: roodask.py, parse_args()

Problem: Both --skip-compile and --force-compile can be specified simultaneously. Their combination is contradictory, and the code gives no error. Whichever branch is checked first (skip_compile is checked before force_compile) will silently "win" — --skip-compile takes priority and --force-compile is silently ignored, which is surprising and potentially confusing.

Fix: Add a mutual-exclusion check after parse_args():

if args.skip_compile and args.force_compile:
    print("ERROR: --skip-compile and --force-compile are mutually exclusive", file=sys.stderr)
    sys.exit(1)

4. Stale config_template.txt references removed concept

File: rooutil/roodask/config_template.txt

The copilot instructions file explicitly states: "config_template field was removed — the script generates the wrapper automatically". However, config_template.txt is still committed. It won't break execution, but it's dead/confusing artifact that the AI generated and forgot to clean up.


⚠️ Minor Issues (non-critical)

  • import math is placed mid-function (after reading the filelist). While valid Python, it's against convention and should be at the top of the file with the other imports.
  • "Error" stderr check is fragile: Any job that prints "Error" to stderr (e.g. a normal ROOT warning that contains the word) will be marked as failed, including messages like TError: no error. A regex check like re.search(r'\bError\b', ...) might be more precise.
  • No validation that source file actually exists before attempting compilation — a missing file would result in a cryptic g++ error rather than a clear message.

The most urgent fix is bug #1 — it will always crash when --hadd is given a relative path. Bug #2 is important for correctness in distributed (non-local) scheduler setups.

@AndrewEdmonds11
Copy link
Copy Markdown
Collaborator Author

Fixed bugs found by Copilot. Also added a validation script with example commands. Merging now...

@AndrewEdmonds11 AndrewEdmonds11 merged commit 215bcaa into Mu2e:main Apr 22, 2026
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.

1 participant