Skip to content

feat: use arrow for datetimes and standardize output block#56

Open
qibinlei wants to merge 18 commits into
usatlas:mainfrom
qibinlei:feat/useArrowForDatetimes
Open

feat: use arrow for datetimes and standardize output block#56
qibinlei wants to merge 18 commits into
usatlas:mainfrom
qibinlei:feat/useArrowForDatetimes

Conversation

@qibinlei
Copy link
Copy Markdown
Contributor

@qibinlei qibinlei commented May 3, 2026

  • Created a new file benchmark_utils.sh which has a function that appends a benchmark block into all output logs
  • Updated parser to use arrow package in python
  • Updated parser to scrape benchmark block (reducing amount of handlers needed)

@qibinlei qibinlei changed the title Feat/use arrow for datetimes feat: use arrow for datetimes and standardize output block May 3, 2026
kratsg
kratsg reviewed May 4, 2026
View reviewed changes
Comment thread EVNT/BNL/CentOS7/centos_cron.sh
kratsg
kratsg reviewed May 4, 2026
View reviewed changes
Comment thread parsing/handlers/base_parser.py Outdated
"runTime": run_time,
"status": status,
"submitTime": start_dt.int_timestamp * 1000, # milliseconds
"queueTime": 0,
Copy link
Copy Markdown
Member

@kratsg kratsg May 4, 2026

Choose a reason for hiding this comment

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

Suggested change
"queueTime": 0,
"queueTime": queueTime,

kratsg
kratsg reviewed May 4, 2026
View reviewed changes
Comment thread parsing/handlers/base_parser.py Outdated
"status": status,
"submitTime": start_dt.int_timestamp * 1000, # milliseconds
"queueTime": 0,
"runTime": wall_time,
Copy link
Copy Markdown
Member

@kratsg kratsg May 4, 2026

Choose a reason for hiding this comment

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

why is this wall_time and not end_time_utc - start_time_utc? The logic should be inside the parsing.

kratsg
kratsg reviewed May 4, 2026
View reviewed changes
Comment thread Rucio/rucio_script.sh
# shellcheck disable=SC1091
# shellcheck disable=SC2115
source "${ATLAS_LOCAL_ROOT_BASE}"/user/atlasLocalSetup.sh -c el9 -m "${2}" -r "export RUCIO_ACCOUNT=jroblesg && \
source "${ATLAS_LOCAL_ROOT_BASE}"/user/atlasLocalSetup.sh -c el9 -m "${2}" -r "export RUCIO_ACCOUNT=qlei && \
Copy link
Copy Markdown
Member

@kratsg kratsg May 4, 2026

Choose a reason for hiding this comment

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

need @qibinlei 's voms credentials added as secrets to af-benchmarking

@kratsg kratsg force-pushed the feat/useArrowForDatetimes branch from b543db4 to 35b246d Compare May 4, 2026 23:01
@kratsg kratsg changed the base branch from feat/useArrowForDatetimes to main May 4, 2026 23:03
kratsg and others added 13 commits May 4, 2026 16:04
@kratsg
bump
be4554d
@claude
fix: compute runTime from timestamps and add benchmark_utils.sh
6ab90f0 
- Replace wall_time_sec with end_time_utc - start_time_utc for runTime
- Use queue_time variable instead of hardcoded 0 for queueTime
- Add benchmark_utils.sh shared shell utilities for benchmark scripts

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
refactor: remove wall_time from benchmark scripts
758da7b 
append_benchmark now takes (log_file, start_time, end_time, mode) —
wall_time_sec is no longer written to the BENCHMARK block since
parsers derive run time from end_time_utc - start_time_utc directly.

Removes start_epoch, end_epoch, and wall_time variables from all
21 run scripts and drops the wall_time_sec field from benchmark_utils.sh.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
refactor: flatten parsing module, remove retired parsers
c78f2de 
All job types now use the BENCHMARK block approach via base_parser.py.
Remove handlers/ subdirectory and all per-type parsers (evnt, truth3,
rucio, coffea, eventloop, fastframes), ParsingClass from base/, and
text_utils.py which had no remaining callers.

Move base_parser.py to parsing/ root and update ci_parse.py import.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
test: add tests for parse_benchmark_block and parse_atlas_log
ec31ae8 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
feat: add script to compare histogram outputs across frameworks
262fa34 
Loads coffea, eventloop, and fastframes photon pT histograms and prints
an integral/ratio summary and saves an overlay plot with a ratio panel.

Highlights the key difference: coffea/eventloop apply an event-level
tightID cut while fastframes fills underflow for events with no tightID
photon via sorted ph1_pt1_NOSYS.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
refactor: rewrite compare_outputs.py using ROOT instead of matplotlib
46c4bea 
Replace uproot + numpy + matplotlib with PyROOT — available on all
ATLAS analysis facilities without extra installs. Output default
changed to PDF (vector, better for publication).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
feat: add setup and queue timing to all benchmark scripts
3828508 
- benchmark_utils.sh: reorder append_benchmark params so mode is last
  (new signature: log_file start end [setup_start] [setup_end] [mode])
  and read SUBMIT_TIME from env for queue-time tracking via HTCondor
- base_parser.py: parse submit_time_utc (→ submitTime ms, queueTime s)
  and setup_start/end_time_utc (→ setupTime s) using arrow
- payload.schema.json: add optional setupTime field
- All 21 run scripts (EVNT, TRUTH3, NTuple_Hist, Rucio):
  - Container scripts: capture setup_start before export
    ATLAS_LOCAL_ROOT_BASE, write SETUP_COMPLETE marker inside -r string
    after asetup/lsetup, grep it after container exits for setup_end
  - Native scripts: capture setup_start/setup_end around
    atlasLocalSetup + asetup/lsetup before payload command
  - Rucio: records setupTime=0 (start_time used for both bounds)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@pre-commit-ci
style: pre-commit fixes
5ff4539
@claude
fix: clamp queueTime and setupTime to zero on clock skew
2227e2c 
Slight clock differences between submit host and worker node can
produce negative values from timestamp arithmetic, violating the
schema's minimum: 0 constraint. Added max(0, ...) guards for both.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
fix: update BNL benchmark path to AF-Benchmarking
7321344 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@pre-commit-ci
style: pre-commit fixes
bf36ca9
@claude
fix: suppress SC1091 shellcheck warnings in BNL batch scripts
242285b 
The sourced benchmark_utils.sh uses an absolute cluster path that
doesn't exist locally, causing false SC1091 failures in shellcheck.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@qibinlei qibinlei requested a review from kratsg May 21, 2026 01:34
Qi Bin Lei and others added 3 commits May 21, 2026 15:28
@claude
fix: align BNL .sub Executable paths with AF_BENCH_DIR install location
27090cd 
The 11 HTCondor .sub files pointed Executable at
/usatlas/u/qlei/dev/af-benchmarking/... while every BNL shell script
sourced and referenced /usatlas/u/qlei/AF-Benchmarking/. This mismatch
meant HTCondor launched executables from a different checkout than the
one the scripts internally relied on.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
test: verify PreToolUse check-typos hook fires on commit
0e12152
Revert "test: verify PreToolUse check-typos hook fires on commit"
de4ca61 
This reverts commit 0e12152.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kratsg kratsg Awaiting requested review from kratsg

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@qibinlei @kratsg