Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ci): run_material_map_validation.sh use v33.1.0 scripts #779

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

This PR updates the version of the scripts used for material map validation to the version of ACTS that is used in the environment.

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?

No.

@github-actions github-actions bot added the topic: infrastructure Regarding build system, CI, CD label Sep 11, 2024
@wdconinc
Copy link
Contributor Author

Succeeds now and looks fine in artifacts at https://github.com/eic/epic/actions/runs/10946548910/job/30393563281?pr=779. The FPE does not seem to make the entire workflow invalid.

@wdconinc wdconinc requested review from veprbl and rahmans1 September 19, 2024 18:51
@veprbl veprbl enabled auto-merge September 27, 2024 08:16
@veprbl veprbl added this pull request to the merge queue Oct 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 9, 2024
@wdconinc
Copy link
Contributor Author

wdconinc commented Oct 9, 2024

@veprbl The geoviewers are failing since we don't have a build_detector phase. I'm adapting the geoviewer jobs to use /opt/detectors/epic-main, see https://eicweb.phy.anl.gov/EIC/benchmarks/geoviewer/-/merge_requests/4. This will require that we trigger the geoviewer job from the container build, and not directly from here. Or did you have a different idea for this?

@wdconinc wdconinc merged commit 335a7f7 into main Oct 9, 2024
81 of 115 checks passed
@wdconinc wdconinc deleted the material-map-validation-acts-33.1.0 branch October 9, 2024 18:53
@wdconinc
Copy link
Contributor Author

wdconinc commented Oct 9, 2024

Force merged 🔨

@veprbl
Copy link
Member

veprbl commented Oct 9, 2024

@veprbl The geoviewers are failing since we don't have a build_detector phase. I'm adapting the geoviewer jobs to use /opt/detectors/epic-main, see https://eicweb.phy.anl.gov/EIC/benchmarks/geoviewer/-/merge_requests/4. This will require that we trigger the geoviewer job from the container build, and not directly from here. Or did you have a different idea for this?

Yes. That would be in line with our current best approaches. My first reaction was to revert https://eicweb.phy.anl.gov/EIC/benchmarks/common_bench/-/merge_requests/91. In the long term, geoviewer should be just another detector benchmark, so it would do good for us to exercise it to run via a container build.

@wdconinc
Copy link
Contributor Author

wdconinc commented Oct 9, 2024

I think all we'd lose in the short term is the links to geo viewers on GitHub since we don't pass the GitHub CI tokens to post external statuses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Regarding build system, CI, CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants