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

SimCore submodule at v1.8.3 (LHE reader generalization) #1271

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Mar 13, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves #1270

To ensure backward compatibility I ran on https://github.com/LDMX-Software/ldmx-sw/blob/trunk/.github/validation_samples/ecal_pn/config.py
--> runs fine

Then I ran on an LHE which has the v3 format, and the original error of the "Wrong number of tokens in LHE particle record." is gone.

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.
  • I attached any sub-module related changes to this PR.

Related Sub-Module PRs

LDMX-Software/SimCore#114

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

Awesome 🙌 thank you for going through the double-PR rigamarole.1

The general strategy I have is to keep submodules on tagged releases, so the procedure described online is

  1. Merge the submodule PR
  2. Create a submodule release for that fix
  3. Double check (update if need be) that the commit reference here is that tag
  4. Merge this PR

I'm now realizing that we may have too strict requirements on review to make this process functional without admin intervention (since I think releases require admin access to that git repo). Maybe a side project here could be asking you to see what permissions you have in SimCore: can you merge your PR after my one review? could you make a new release?

Anyways, the changes I'm requesting are to follow the above procedure. Ideally, since this PR only consists of updating the submodule, this PR would just be one commit with the message SimCore on <version> or something like that.

Footnotes

  1. Any ideas on how to make it smoother/easier to contribute to submodules welcome.

@tvami
Copy link
Member Author

tvami commented Mar 15, 2024

asking you to see what permissions you have in SimCore: can you merge your PR after my one review?

It needs two reviews, I could give it a try and add my own review :P and see if that works.

this PR would just be one commit with the message SimCore on <version>

I could rebase/reword the commit on this PR, no problem.

Any ideas on how to make it smoother/easier to contribute to submodules welcome.

My understanding is that the submodules were motivated because there was a lot of development happening in paralell, now that the software is in a more stable part, I'm wondering if this still makes sense or we should just merge everything into one big module (like CMSSW). [also this is beyond this PR, maybe we should have a separate issue about this (or just kill my suggestion now and that also resolves it hahaha)]

@tvami tvami changed the title Generalize LHEReader so it can read LHE v3.0 input as well SimCore submodule at v1.8.3 (LHE reader generalization) Mar 18, 2024
Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

Thank you for re-wording the commit! Unfortunately, our submodule-based workflow is even more complicated 😮‍💨

SimCore is not on the v1.8.3 tag (although it is on a commit with identical source code).

tom@appa:~/code/ldmx/ldmx-sw$ git branch -vv
  1232-use-denv-in-ldmx-env 312a58d4 [origin/1232-use-denv-in-ldmx-env: behind 13] auto-install denv so its so light, add printouts to transition
* iss1270                   539e0446 [origin/iss1270] SimCore submodule at v1.8.3 (LHE reader generalization)
  patch-module-selection    18059af4 only build event library corresponding to requested modules
  trunk                     39fc257c [origin/trunk: behind 16] Bump ldmx/dev from 4.2.0 to 4.2.1
tom@appa:~/code/ldmx/ldmx-sw$ git -C SimCore describe --tags
v1.8.2-1-g11b0e48
tom@appa:~/code/ldmx/ldmx-sw$ git -C SimCore diff v1.8.3
tom@appa:~/code/ldmx/ldmx-sw$ 

This reason for this is because a new commit was created when merging the submodule PR:

tom@appa:~/code/ldmx/ldmx-sw/SimCore$ git log --graph --oneline --all
* 191f2ec (tag: v1.8.3, origin/trunk, origin/HEAD) Generalize LHEReader so it can read LHE v3.0 input as well
| * 11b0e48 (HEAD, origin/iss1270) Generalize LHEReader so it can read LHE v3.0 input as well
|/  
* c8a30c1 (tag: v1.8.2) enable all .py files in test to be tests

The solution for this is to intentionally checkout the tag created by the SimCore release.

tom@appa:~/code/ldmx/ldmx-sw$ git -C SimCore fetch --tags && git -C SimCore checkout v1.8.3
Previous HEAD position was 11b0e48 Generalize LHEReader so it can read LHE v3.0 input as well
HEAD is now at 191f2ec Generalize LHEReader so it can read LHE v3.0 input as well
tom@appa:~/code/ldmx/ldmx-sw$ git -C SimCore fetch --tags && git -C SimCore checkout v1.8.3
Previous HEAD position was 11b0e48 Generalize LHEReader so it can read LHE v3.0 input as well
HEAD is now at 191f2ec Generalize LHEReader so it can read LHE v3.0 input as well
tom@appa:~/code/ldmx/ldmx-sw$ git -C SimCore describe --tags
v1.8.3

Then you could re-make the commit for this PR:

git add SimCore
git commit --amend --no-edit

(--no-edit because I like the wording it currently has)

@tvami tvami force-pushed the iss1270 branch 2 times, most recently from 9be638e to a4e7010 Compare March 19, 2024 15:57
Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

Looks good, updating SimCore to the new release tag:

tom@appa:~/code/ldmx/ldmx-sw$ git submodule status
 4056cf8684bd6eea0e13e4ab3e24958ccf8124e3 Conditions (v0.1.0)
 c92f48575ab1e4ea90c1b14c2c414727383a9ce6 Ecal (v0.2.6)
 1eb1fba1bfe8a88a29947524e14fd9ed2dd89e6a Framework (v1.4.2)
 ac43c59c00adb1fad16aa6b7a63b0386ef8ebe43 Hcal (v0.3.6)
 08e5aa5c72fb82fe3ce123fff0c6483610c10ba6 MagFieldMap (v0.1.0)
 191f2eccfefb20f293f9c2e8ab5e89ec3da1d9aa SimCore (v1.8.3)
 a1a9ec8e51cf9c22e8be788e6d7fbf7235239902 Tracking (v1.2.1)
 cb60589597edea854031594e25ead4a37fc1c829 TrigScint (remotes/origin/iss53-4-gcb60589)
 c5731f01be27233c166fc7ff416677a0d95f4b87 Trigger (v0.1.0)
 f3a9248f28597ccdc2542e142d06f4fb31dd92f7 cmake (v0.4.0)
 df83fbf22cfff76b875c13d324baf584c74e96d0 docs/doxygen.conf/doxygen-awesome-css (v2.2.1)

@tomeichlersmith tomeichlersmith merged commit 2e3ab12 into trunk Mar 19, 2024
1 check passed
@tomeichlersmith tomeichlersmith deleted the iss1270 branch March 19, 2024 18:52
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.

Support LHE v3.0
2 participants