Skip to content

Enhancements/Fixes(2026.3.0) Sprint 1 - #254

Open
sumanaga wants to merge 9 commits into
intel-retail:mainfrom
sumanaga:new-changes
Open

sumanaga wants to merge 9 commits into
intel-retail:mainfrom
sumanaga:new-changes

Conversation

@sumanaga

@sumanaga sumanaga commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

PR Checklist

  • Added label to the Pull Request for easier discoverability and search
  • Commit Message meets guidelines as indicated in the URL https://github.com/intel-retail/performance-tools/blob/main/CONTRIBUTING.md
  • Every commit is a single defect fix and does not mix feature addition or changes
  • Unit Tests have been added for new changes
  • Updated Documentation as relevant to the changes
  • All commented code has been removed
  • If you've added a dependency, you've ensured license is compatible with repository license and clearly outlined the added dependency.
  • PR change contains code related to security
  • PR introduces changes that breaks compatibility with other modules (If YES, please provide details below)

What are you changing?

  1. Stream density: define the measurement window in seconds, and delete the unused fail threshold #250
    Removed faill threshould and Updated stream-density benchmarking to use an explicit measurement window in seconds for each density step, with consecutive windows used to confirm pass/fail results. Removed the unused fail-threshold configuration and related handling. Documentation now explains MEASUREMENT_WINDOW_SECONDS, INIT_DURATION, target FPS resolution, and how these settings affect benchmark duration and results. The final report includes the settle time and measurement-window details for clearer result interpretation.
  2. Stream density: report the rate that was measured and the rule that was applied [Stream density: report the rate that was measured and the rule that was applied #247]
    1. The stream-density benchmark resolves target FPS in this order:
      TARGET_FPS environment variable: overrides the target for every stream.
      Per-camera targetFps in the selected camera_to_workload_*.json.
      Per-camera fps in the camera configuration.
      Default target of 14.95 FPS if neither JSON field contains a positive value.
      For example, if TARGET_FPS=12 is set, every stream uses 12 FPS, even if its camera has targetFps: 15. Without the environment override, each camera can use its own targetFps; otherwise its source fps is used as the fallback.
      2. Updated stream-density reports to show the measured FPS for each stream alongside its target FPS and pass threshold. The final summary identifies the stream with the lowest throughput or headroom and reports the exact rule used to determine success. Results now clearly distinguish uniform-target runs from runs with individual per-stream targets. This makes the benchmark outcome easier to audit and interpret.
      3. For different targets, report logic is as below:
      For each stream, the benchmark calculates:
      pass mark = target FPS × pass tolerance ratio
      relative headroom = (measured FPS − pass mark) ÷ pass mark
      When streams have different targets, the stream with the smallest relative headroom is reported as the lowest- headroom stream:
      cam1: target 15, pass mark 14.25, measured 15.00
      headroom = (15.00 - 14.25) / 14.25 = 5.3%
      cam2: target 10, pass mark 9.50, measured 9.60
      headroom = (9.60 - 9.50) / 9.50 = 1.1%
      Although cam1 has the larger absolute margin (0.75 FPS versus 0.10 FPS), cam2 has less relative headroom and is therefore reported as the limiting stream. All streams must still meet their own individual pass marks for the overall result to pass.

Sample report output for same targets:
1. Beginning of the log
INFO: Stream density per-camera target FPS configuration: cam1=15.00 FPS from fps, cam2=15.00 FPS from fps with container_name gst0 and INIT_DURATION set for 60 seconds; measurement window 30 seconds; required agreement 2 pass / 2 fail windows
2. End of the log

Stream density result

Streams sustained 36
Settle time 60 s (INIT_DURATION before measuring)
Measurement window 30 s per window; 2 consecutive windows must agree
(each window is measured on its own; the count moves
only after 2 windows in a row agree - windows are not added together)
Pass mark target x 0.95 (pass tolerance ratio)

stream camera workload target pass mark measured result
pipeline_stream0 cam1 asc_obj_detection_age_prediction_face_detection 15.00 14.25 15.65 pass
pipeline_stream1 cam2 asc_obj_detection_age_prediction 15.00 14.25 15.92 pass
pipeline_stream2 cam1 asc_obj_detection_age_prediction_face_detection 15.00 14.25 16.63 pass

Result: Pass - 36 concurrent streams sustained
The run was evaluated after a 60 second settle time (INIT_DURATION), over 2 consecutive measurement windows of 30 seconds each. All streams met the 14.25 FPS minimum requirement. The lowest-throughput stream was pipeline_stream35 (cam2 (asc_obj_detection_age_prediction)), targeting 15.00 FPS and measuring 14.53 FPS against its 14.25 FPS minimum, leaving 0.28 FPS (2.0% relative headroom).

Sample report output for Different targets
1. Beginning of the log
INFO: Stream density per-camera target FPS configuration: cam1=14.00 FPS from targetFps, cam2=13.00 FPS from targetFps with container_name gst0 and INIT_DURATION set for 60 seconds; measurement window 30 seconds; required agreement 2 pass / 2 fail windows
2. End of the log
Result: Pass - 38 concurrent streams sustained
The run was evaluated after a 60 second settle time (INIT_DURATION),over 2 consecutive measurement windows of 30 seconds each. All streams met their individual minimum FPS requirements. The lowest-headroom stream was pipeline_stream34 (cam1 (asc_obj_detection_age_prediction_face_detection)), targeting 14.00 FPS and measuring 14.74 FPS against its 13.30 FPS minimum, leaving 1.44 FPS (10.8% relative headroom).

Issue this PR will close

close: #250 , #247

Anything the reviewer should know when reviewing this PR?

Test Instructions if applicable

  1. To override default measurement window
    make benchmark-stream-density
    MEASUREMENT_WINDOW_SECONDS=60
  2. To override TARGET_FPS in the configuration file
    make benchmark-stream-density
    TARGET_FPS=13

If the there are associated PRs in other repositories, please link them here (i.e. intel-retail/performance-tools )

intel-retail/loss-prevention#359

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved target override, initialization propagation, stream counting, failure reporting, and test-execution issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 3 Medium severity · 1 Low severity

Open (6)
What changed in this PR

This PR enhances stream-density benchmarking with configurable measurement windows, per-camera FPS targets, and richer reporting.

Changes:

  • Adds timed measurement windows and removes unused hysteresis handling.
  • Reports targets, pass thresholds, measured FPS, and headroom.
  • Updates CLI integration, result handling, and unit tests.
File Summary
benchmark-scripts/​stream_density.py Implements target resolution, windowed measurements, and detailed reporting.
benchmark-scripts/​stream_density_test.py Updates tests for new measurement behavior and return values.
benchmark-scripts/​benchmark.py Adds measurement-window CLI support and revised result handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread benchmark-scripts/stream_density.py Outdated
Comment thread benchmark-scripts/stream_density_test.py Outdated
Comment thread benchmark-scripts/benchmark.py
Comment thread benchmark-scripts/stream_density.py Outdated
Comment thread benchmark-scripts/stream_density.py
Comment thread benchmark-scripts/stream_density.py Outdated
@sumanaga
sumanaga marked this pull request as ready for review September 24, 2026 08:03

@jcork-intel jcork-intel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks Sumana, and thank you for turning this around so quickly. Thanks also for filling in the PR template fully; the description and sample output made this much faster to review. Removing the fail threshold, measuring only inside a fixed window after the settle time, and taking each camera's target from its own fps are all things we asked for, and the new end-of-run summary, with the per-stream table and the pass rule written out, is much easier to read. I ran stream_density_test.py locally at the latest commit and all 11 tests pass. They cover the per-camera targets and the stream count, but not the two main changes, the measurement window and the new summary. The Copilot findings look fixed in the second round of commits.

I'd like three changes before this merges, because each one changes a number or a word a customer sees.

  1. The "measured" column shows the 90th percentile, not the measured rate. "Report the rate that was measured and the rule that was applied" (#247) asks for the measured per-stream rate next to the target, and this PR closes #247, so this is in scope. In the sample report, 15 fps cameras show measured 16.63 and 15.92. A camera can't deliver more frames than its source sends, so a customer reading that table will think the tool is broken.

    Here is an example of the same result written both ways, with illustrative values:

    As this PR writes it:

    stream             target  pass mark  measured  result
    pipeline_stream0    15.00      14.25     16.0     pass
    pipeline_stream1    15.00      14.25     17.0     pass
    pipeline_stream2    15.00      14.25     16.9     pass
    pipeline_stream3    15.00      14.25     16.9     pass
    
    The lowest-throughput stream was pipeline_stream0, ... measuring 16.0 FPS
    against its 14.25 FPS minimum, leaving 1.75 FPS (12% relative headroom).
    

    What I'd like:

    stream             target  pass mark  measured (avg)  p90    seconds below pass mark  result
    pipeline_stream0    15.00      14.25           14.6   16.0    40%                      pass
    pipeline_stream1    15.00      14.25           14.8   17.0    40%                      pass
    pipeline_stream2    15.00      14.25           14.7   16.9    40%                      pass
    pipeline_stream3    15.00      14.25           14.3   16.9    50%                      pass
    
    The lowest-throughput stream was pipeline_stream3, ... measuring 14.3 FPS
    (frames delivered / measurement time) against its 14.25 FPS minimum,
    leaving 0.05 FPS (under 1% relative headroom).
    

    The first version can name the wrong stream as the slowest, and it reports plenty of headroom at a lane count where the next lane up fails. The second version shows the box is at its limit. Please make "measured" the average over the window (frames divided by time), and show p90 and the share of seconds below the pass mark next to it. That also meets #247's requirement that every number in the result can be worked out from the raw log. Changing the pass/fail rule itself from p90 to the average is "Frames over measurement time" (#245). I'm confirming that with the EDO team this week, so this PR doesn't need to change the rule.

  2. A 30 second window is too few samples. It gives about 30 one-second samples per stream. We agreed with Xintian on at least 100 samples per stream after the warmup. In our internal testing, at a lane count right at the limit, a stream's average varied noticeably between short blocks of samples, and it stopped getting steadier at about 100 samples. Please make the default window 100 seconds, or add a minimum of 100 samples. I haven't sent the team the outcome of my discussion with Xintian yet, so this is the first you're hearing of the 100-sample minimum. I'll send a written summary of what we agreed later today.

  3. The settle time can be much shorter. Switching from the 30% ratio to a fixed settle time is the right change. In our internal testing almost every stream was steady within a few seconds, so a 60 to 120 second settle mostly adds time to every step of a density run. We're proposing 10 seconds. The defaults Loss Prevention users actually get are set in the loss-prevention Makefile in PR #359 (intel-retail/loss-prevention#359): INIT_DURATION ?= 60 and MEASUREMENT_WINDOW_SECONDS ?= 30. I'll ask for the same values there, a 10 second settle and a 100 second window.

Three smaller things:

  • Please add tests for the measurement window (samples from before the window start or after its end are left out) and for the summary (target, pass mark and measured rate appear as separate numbers). test_calculate_multi_stream_fps_success also asserts the stream value is the top sample (the p90 rule), so it will need updating when "measured" becomes the average.
  • No CI workflow runs stream_density_test.py, so these tests only help if someone runs them by hand. Can we add them to a workflow, in this PR or a follow-up?
  • All six commits are titled "Update .py". A one-line description per commit would make the history readable.

This changes how the published density number is produced, so please do a before-and-after run: the stock LP lane on a Panther Lake box, with main and with this branch, showing the lane count and each stream's measured FPS. Because this repo is public and benchmark results need legal approval before they're published, please don't post the numbers here. Email them to me, or put them in SharePoint and link to that from the PR. I'll spot-check one point on our side.

@jcork-intel

Copy link
Copy Markdown

One more for this PR, and it came from the template. The "Updated Documentation" box was unchecked, which made me ask what this change does to the docs we already have, so I went looking.

The public docs for benchmark.py and the LP stream density settings are in intel-retail/documentation: docs/user-guide/documentation/benchmark.md and docs/user-guide/loss-prevention/lp-benchmarking.md. Both will be out of date once this merges: there's no --measurement_window_seconds, --init_duration shows the old default, and the target FPS fallback order is the old one. Could you open a PR there alongside this one? I've put the details in my review on loss-prevention#359.

I'm glad the team is using the template now; it's what caught this. It helps most when each item gets a real look, with a short note on any box left unchecked saying why it doesn't apply (for example, "no docs affected" or "docs PR to follow: <link>").

@jcork-intel

Copy link
Copy Markdown

On commit messages: this repo's CONTRIBUTING.md asks for Conventional Commits. Before merge, could the six Update <file>.py commits be reworded, or squashed into one with a conventional message, for example feat(stream-density): measure inside a fixed window and drop the unused fail threshold? PRs here merge with merge commits, so these titles go into main's history as they are, and "Update stream_density.py" only repeats the file list.

@jcork-intel

Copy link
Copy Markdown

Correction to my follow-up about the docs: the live page is docs_src/performance-tools/benchmark.md, not the docs/user-guide/ copy. Details on loss-prevention#359.

Return per-stream FPS samples from calculate_multi_stream_fps and surface
them in the density report: add a p90 column and a seconds-below-pass-mark
column so a stream's stability is visible, not just its average, plus a
Lanes sustained line alongside Streams sustained.

Replace count_pipeline_scaled_streams with count_valid_streams, dropping
the num_pipelines multiplier that inflated the sustained-stream count;
pipeline indices already span every lane. Update unit tests to match the
new return arity and report format.

Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
Align the stream_density fallback defaults with the argparse defaults and
the Makefile: INIT_DURATION falls back to 10 seconds instead of 120, and
DEFAULT_MEASUREMENT_WINDOW_SECONDS is 100 instead of 30. Update the
validate_and_setup_env unit tests to expect the 10 second fallback.

Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
Align the stream_density fallback defaults with the argparse defaults and
the Makefile: INIT_DURATION falls back to 10 seconds instead of 120, and
DEFAULT_MEASUREMENT_WINDOW_SECONDS is 100 instead of 30. Update the
validate_and_setup_env unit tests to expect the 10 second fallback.

Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
@sumanaga

sumanaga commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks Sumana, and thank you for turning this around so quickly. Thanks also for filling in the PR template fully; the description and sample output made this much faster to review. Removing the fail threshold, measuring only inside a fixed window after the settle time, and taking each camera's target from its own fps are all things we asked for, and the new end-of-run summary, with the per-stream table and the pass rule written out, is much easier to read. I ran stream_density_test.py locally at the latest commit and all 11 tests pass. They cover the per-camera targets and the stream count, but not the two main changes, the measurement window and the new summary. The Copilot findings look fixed in the second round of commits.

I'd like three changes before this merges, because each one changes a number or a word a customer sees.

  1. The "measured" column shows the 90th percentile, not the measured rate. "Report the rate that was measured and the rule that was applied" (Stream density: report the rate that was measured and the rule that was applied #247) asks for the measured per-stream rate next to the target, and this PR closes Stream density: report the rate that was measured and the rule that was applied #247, so this is in scope. In the sample report, 15 fps cameras show measured 16.63 and 15.92. A camera can't deliver more frames than its source sends, so a customer reading that table will think the tool is broken.
    Here is an example of the same result written both ways, with illustrative values:
    As this PR writes it:

    stream             target  pass mark  measured  result
    pipeline_stream0    15.00      14.25     16.0     pass
    pipeline_stream1    15.00      14.25     17.0     pass
    pipeline_stream2    15.00      14.25     16.9     pass
    pipeline_stream3    15.00      14.25     16.9     pass
    
    The lowest-throughput stream was pipeline_stream0, ... measuring 16.0 FPS
    against its 14.25 FPS minimum, leaving 1.75 FPS (12% relative headroom).
    

    What I'd like:

    stream             target  pass mark  measured (avg)  p90    seconds below pass mark  result
    pipeline_stream0    15.00      14.25           14.6   16.0    40%                      pass
    pipeline_stream1    15.00      14.25           14.8   17.0    40%                      pass
    pipeline_stream2    15.00      14.25           14.7   16.9    40%                      pass
    pipeline_stream3    15.00      14.25           14.3   16.9    50%                      pass
    
    The lowest-throughput stream was pipeline_stream3, ... measuring 14.3 FPS
    (frames delivered / measurement time) against its 14.25 FPS minimum,
    leaving 0.05 FPS (under 1% relative headroom).
    

    The first version can name the wrong stream as the slowest, and it reports plenty of headroom at a lane count where the next lane up fails. The second version shows the box is at its limit. Please make "measured" the average over the window (frames divided by time), and show p90 and the share of seconds below the pass mark next to it. That also meets Stream density: report the rate that was measured and the rule that was applied #247's requirement that every number in the result can be worked out from the raw log. Changing the pass/fail rule itself from p90 to the average is "Frames over measurement time" (Stream density: decide pass or fail on frames processed divided by measurement time #245). I'm confirming that with the EDO team this week, so this PR doesn't need to change the rule.
    [Made this change: Added p90, seconds below pass mark]
    Didn't change the pass/fail rule as yet to get the confirmation

stream            camera  workload                                            target  pass mark       p90   seconds below pass mark  result
pipeline_stream0  cam1    asc_obj_detection_age_prediction_face_detection      15.00      14.25     15.94              62/100 (62%)    pass
  1. A 30 second window is too few samples. It gives about 30 one-second samples per stream. We agreed with Xintian on at least 100 samples per stream after the warmup. In our internal testing, at a lane count right at the limit, a stream's average varied noticeably between short blocks of samples, and it stopped getting steadier at about 100 samples. Please make the default window 100 seconds, or add a minimum of 100 samples. I haven't sent the team the outcome of my discussion with Xintian yet, so this is the first you're hearing of the 100-sample minimum. I'll send a written summary of what we agreed later today.
    [Made the default value changes]
  2. The settle time can be much shorter. Switching from the 30% ratio to a fixed settle time is the right change. In our internal testing almost every stream was steady within a few seconds, so a 60 to 120 second settle mostly adds time to every step of a density run. We're proposing 10 seconds. The defaults Loss Prevention users actually get are set in the loss-prevention Makefile in PR #359 (intel-retail/loss-prevention#359): INIT_DURATION ?= 60 and MEASUREMENT_WINDOW_SECONDS ?= 30. I'll ask for the same values there, a 10 second settle and a 100 second window.
    [Made the default value changes]

Three smaller things:

  • Please add tests for the measurement window (samples from before the window start or after its end are left out) and for the summary (target, pass mark and measured rate appear as separate numbers). test_calculate_multi_stream_fps_success also asserts the stream value is the top sample (the p90 rule), so it will need updating when "measured" becomes the average.
    [Implemented in this PR]
  • No CI workflow runs stream_density_test.py, so these tests only help if someone runs them by hand. Can we add them to a workflow, in this PR or a follow-up?
    [Will create a task, targeted Sprint 3]
  • All six commits are titled "Update .py". A one-line description per commit would make the history readable.
    > [Will be done when merging using squash and merge option]
    This changes how the published density number is produced, so please do a before-and-after run: the stock LP lane on a Panther Lake box, with main and with this branch, showing the lane count and each stream's measured FPS. Because this repo is public and benchmark results need legal approval before they're published, please don't post the numbers here. Email them to me, or put them in SharePoint and link to that from the PR. I'll spot-check one point on our side.

@jcork-intel jcork-intel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks Sumana, and for answering each point inline. The 10 s warmup period, the 100 s measurement interval, and the p90 and seconds-below report all look good. So does the target order: TARGET_FPS only when someone passes it, then targetFps, then the camera's fps, which is what I asked for in my 16 Sep correction.

A few items before I approve:

  1. Pass rule: on 24 Sep I asked you to hold this until it was confirmed. It's now committed (with the benchmarking team on 25 Sep, and again on the 29 Sep call), so let's move forward with it in this PR. Please switch the per-stream decision from p90 to total frames divided by the measurement interval, and keep p90 in the report.

  2. Average column: please add the per-stream average (frames divided by time) as "measured (avg)" next to p90 and seconds below. What I asked for on 24 Sep, what the report shows today, and what it should show with the agreed terms (point 5) and p10 (point 3):

    Asked for (24 Sep review)
    stream   target  pass mark  measured (avg)    p90  seconds below  result
    stream0   15.00      14.25            14.6  16.00            40%  pass
    stream3   15.00      14.25            14.3  16.90            50%  pass
    
    Report today (your 29 Sep sample)
    stream   target  pass mark  measured (avg)    p90  seconds below pass mark  result
    stream0   15.00      14.25       (missing)  15.94             62/100 (62%)  pass
    
    Should be (agreed terms and columns, your sample)
    stream   target  throughput threshold  measured (avg)    p10    p90  seconds below threshold  result
    stream0   15.00                 14.25           (avg)  (p10)  15.94             62/100 (62%)  (from avg)
    

    In your sample, pipeline_stream0 was below 14.25 fps 62% of the time, and on p90 alone it reads as a comfortable pass.

  3. p10: as discussed on the 29 Sep call, please report p10 alongside p90.

  4. Minimum samples: the 100 s default gives about 100 samples. How is the 100-sample minimum enforced if someone sets a shorter measurement interval?

  5. Terminology: this one is on us. My 24 Sep review used "pass mark" and "settle time", and we've since agreed one vocabulary with the benchmarking team. Please use these words in the report, logs and help text (variable and env names can stay as they are):

    • "pass mark" becomes "throughput threshold" (column header, "seconds below throughput threshold", and the summary sentence)
    • "settle time" becomes "warmup period"
    • "measurement window" becomes "measurement interval"
    • "consecutive pass windows" and "required agreement" become "run acceptance criterion (two consecutive passes)"
    • "Lanes sustained" becomes "Use case density (lanes)", and "Streams sustained" becomes "Stream density (streams)"
  6. Stream count: thanks for dropping the pipeline-count multiplier in count_valid_streams. Your comment is right that pipeline.sh already carries every lane's sources; our 5-lane run on 22 Sep produced 30 per-stream logs, so the old version would have reported 150. Could you add a unit test that pins 5 lanes of 6 cameras to 30 streams, so the multiplier doesn't come back?

  7. Last-resort default: DEFAULT_TARGET_FPS in stream_density.py is still 14.95. Please make it 15. It only applies when a camera has neither targetFps nor fps, but then that camera is judged at 14.25 like every other 15 fps camera, not 14.20.

This branch has not been deployed

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stream density: define the measurement window in seconds, and delete the unused fail threshold

3 participants