Skip to content

Conversation

@simonge
Copy link
Contributor

@simonge simonge commented Jun 4, 2025

Briefly, what does this PR introduce?

Introduces a benchmark which outputs the position and momentum of the electron beam through the outgoing beamline magnets.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

Adds a new benchmark

@simonge simonge force-pushed the beamline_benchmark branch from 931a7e0 to adda7dd Compare June 4, 2025 18:27
@simonge simonge requested a review from veprbl June 18, 2025 19:55
@simonge simonge marked this pull request as ready for review June 18, 2025 19:55
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

No operational concerns from me. Plots look good.

@simonge
Copy link
Contributor Author

simonge commented Jun 19, 2025

Thanks @veprbl. A few questions from me.

The simulation appears to have run twice rather than bench:beamline picking up and using the file created by sim:beamline. Is this correct? How/why is the workflow divided up between the github yml and snakemake?

Locally, when have the other benchmarks in the top Snakefile uncommented, snakemake appears to freeze tying to identify the job graph. e.g. running snakemake --cores 8 results/beamline/local/ just hangs

Are the results being published anywhere at the moment or do we need to download the artefacts ourselves?

@simonge simonge force-pushed the beamline_benchmark branch from 470b303 to 67d4206 Compare June 19, 2025 10:34
@veprbl
Copy link
Member

veprbl commented Jun 19, 2025

Thanks @veprbl. A few questions from me.

The simulation appears to have run twice rather than bench:beamline picking up and using the file created by sim:beamline. Is this correct? How/why is the workflow divided up between the github yml and snakemake?

This is a bug. The issue seems to be that the gps input file has an ever more recent timestamp on each git fetch, and each gitlab jobs starts afresh. We can't pass .snakemake directory from sim step to bench because there are often several sim steps running in parallel and its not obvious how one can merge metadata, we can't have a shared .snakemake directory either. This makes Snakemake go off the timestamps only and mark the simulation rule for rerunning. For most jobs this is not an issue since we use Snakemake caching (you need to specify caching: True and provide params that are used in the cache key). It would be nice to figure this one out properly, though.

Locally, when have the other benchmarks in the top Snakefile uncommented, snakemake appears to freeze tying to identify the job graph. e.g. running snakemake --cores 8 results/beamline/local/ just hangs

That is odd. It starts instantly for me with the current state of repo. It can be slow to compute a large graph, but we should not have any such things in any of the benchmarks. Try with --dry-run --verbose --debug-dag maybe?

Are the results being published anywhere at the moment or do we need to download the artefacts ourselves?

You can click the eicweb/detector_benchmarks (nightly, epic_craterlake) job. There scroll right to either bench:beamline or collect_results:beamline, open that job and there will be a browse button for artifacts. Those are available immediately but stored for a limited amount of time.
Artifacts are also uploaded to JLab for permanent storage, there is a browser https://eic.jlab.org/epic/image_browser.html that displays png images.

@simonge
Copy link
Contributor Author

simonge commented Jun 20, 2025

This is a bug. The issue seems to be that the gps input file has an ever more recent timestamp on each git fetch, and each gitlab jobs starts afresh.

I should probably just remove the gps macro from the inputs of the benchmark when uploading here.

Are the results being published anywhere at the moment or do we need to download the artefacts ourselves?

You can click the eicweb/detector_benchmarks (nightly, epic_craterlake) job. There scroll right to either bench:beamline or collect_results:beamline, open that job and there will be a browse button for artifacts. Those are available immediately but stored for a limited amount of time. Artifacts are also uploaded to JLab for permanent storage, there is a browser https://eic.jlab.org/epic/image_browser.html that displays png images.

I was trying to check the image browser but from what I see it doesn't include any newer pipelines than 117112

@simonge
Copy link
Contributor Author

simonge commented Jun 20, 2025

I should probably just remove the gps macro from the inputs of the benchmark when uploading here.

I see you've already found a better solution.

@veprbl
Copy link
Member

veprbl commented Jun 20, 2025

This is a bug. The issue seems to be that the gps input file has an ever more recent timestamp on each git fetch, and each gitlab jobs starts afresh.

I should probably just remove the gps macro from the inputs of the benchmark when uploading here.

Don't do that. This tooling is mean to be used not just on CI, so we want to rerun the simulation whenever one changes its parameters.

Are the results being published anywhere at the moment or do we need to download the artefacts ourselves?

You can click the eicweb/detector_benchmarks (nightly, epic_craterlake) job. There scroll right to either bench:beamline or collect_results:beamline, open that job and there will be a browse button for artifacts. Those are available immediately but stored for a limited amount of time. Artifacts are also uploaded to JLab for permanent storage, there is a browser https://eic.jlab.org/epic/image_browser.html that displays png images.

I was trying to check the image browser but from what I see it doesn't include any newer pipelines than 117112

cc @torrijeske regarding that

@simonge simonge merged commit 2ce151d into master Jun 23, 2025
3 checks passed
@simonge simonge deleted the beamline_benchmark branch June 23, 2025 21:43
@simonge
Copy link
Contributor Author

simonge commented Jun 23, 2025

I was trying to check the image browser but from what I see it doesn't include any newer pipelines than 117112

cc @torrijeske regarding that

Thanks @veprbl. It appears to be more recent now but not obviously updating automatically.

@torrijeske
Copy link

working on it -- sorry about that.

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.

4 participants