search

SegmentLinking / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1 stars 1 forks source link

Add LST and LSTCore #30

Open ariostas opened 1 month ago

ariostas commented 1 month ago

Here's a draft of the LSTCore package, but now using real files. Here are some notes about it.

I tested running both standalone and CMSSW, and everything seems fine.

ariostas commented 1 month ago

This is a replacement of #29. This is still a draft since it includes all the data files, which should be removed. There's also many files that were added just for the standalone part, and I'm not sure if that's fine.

VourMa commented 1 month ago

Will check it out tonight and maybe test it and report back. Thanks!

slava77 commented 1 month ago

The data directory is temporary and should be removed before we submit the PR to the main CMSSW repo. We should get started populating RecoTracker-LSTCore.

these will likely trigger an issue of increasing the git repo by too much and will need to be removed from the git history.

Is it a matter of getting CI to know where to take the data files? We could already populate the master in a fork of cms-data in https://github.com/SegmentLinking/RecoTracker-LSTCore and just have the CI do git clone https://github.com/SegmentLinking/RecoTracker-LSTCore.git $CMSSW_BASE/src/RecoTracker/LSTCore/data

ariostas commented 1 month ago

these will likely trigger an issue of increasing the git repo by too much and will need to be removed from the git history.

Yeah, I removed them from the git history

Is it a matter of getting CI to know where to take the data files?

Yeah, I'll do that. Thanks!

VourMa commented 1 month ago

I also tried the standalone version within CMSSW. It compiles fine but it doesn't run because:

ERROR: Could not find the file = "/home/users/evourlio/LSTinCMSSW/movingFullyToCMSSW/CMSSW_14_1_0_pre3/src/RecoTracker/LSTCore/standalone/..//data/OT800_IT615_pt0.8/endcap_orientation.bin"

I guess this is because data files were removed from the history. Are there changes pending for things to work? Or is there some manual intervention required, like I need to clone the data files with an extra command? ~I see that this is still empty, so I can just push the data folder structure there?~ I noticed that Andres has already created an initial branch there, that is filled with data.

slava77 commented 1 month ago

I also tried the standalone version within CMSSW. ...

we should probably decide what "standalone" would actually mean:

I guess this is because data files were removed from the history. Are there changes pending for things to work?

That's a bit related to the above:

slava77 commented 1 month ago
  • we can default to what will be visible in $CMSSW_RELEASE_BASE/external/el8_amd64_gcc12/data/RecoTracker/LSTCore/data

and if that path is not available, to add a git clone/checkout from https://github.com/SegmentLinking/RecoTracker-LSTCore

VourMa commented 1 month ago

we should probably decide what "standalone" would actually mean:

  • a setup that can compile and run sdl_cuda and such while also being able to run a cmsRun
  • or something more "restrictive" (with lower dependency stack) using a sparse checkout where only the libsdl* and sdl* bins are compiled and can run.

In principle both can work with small changes to the current state of the code, I believe.

For the first case, all we need is to modify: https://github.com/SegmentLinking/cmssw/blob/8ced0f1ba0b4c03a8da3074d029a3104b9836b15/RecoTracker/LSTCore/src/alpaka/LSTESData.dev.cc#L14-L30 to point to a different directory according to your comment:

and if that path is not available, to add a git clone/checkout from https://github.com/SegmentLinking/RecoTracker-LSTCore

For the second case, I did:

git clone --filter=blob:none --no-checkout --depth 1 --sparse --branch CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles https://github.com/SegmentLinking/cmssw.git
cd cmssw/
git sparse-checkout add HeterogeneousCore/AlpakaInterface RecoTracker/LSTCore
git checkout

Given the absence of the $CMSSW_RELEASE_BASE/external/el8_amd64_gcc12/data/RecoTracker/LSTCore/data directory, I did:

cd RecoTracker/LSTCore
git clone git@github.com:SegmentLinking/RecoTracker-LSTCore.git data
cd data
git fetch origin initial
git checkout initial
cd ../../..

Then, the include directories are a bit off, because there is no proper $CMSSW_BASE. I fixed this by including -I../../.. or -I../../../.. in the Makefile and SDL/Makefile respectively. Doing so, I managed to run everything. Of course, this was a "hack" at this stage. The proper solution to me would be to add a compilation flag to disable the LST_IS_CMSSW_PACKAGE flag, and change lines like: https://github.com/SegmentLinking/cmssw/blob/8ced0f1ba0b4c03a8da3074d029a3104b9836b15/RecoTracker/LSTCore/src/alpaka/Kernels.h#L7-L10 to point to the appropriate directories, as these lines are useless right now, if also the standalone is in CMSSW.

UPDATE:

As of 6025567, the fix of the Makefiles has been applied, so the standalone package can be used without manual interventions.

ariostas commented 1 month ago

I foresee a comment at the review about the many commits with "invalid states"

Yeah, I think it makes sense to squash commits further. Let's also resolve all the discussions here by adding more commits to this branch and once it's in good shape we'll squash everything into 1-2 commits.

ariostas commented 1 month ago

Here's a new branch with squashed commits. I can change the branch name and/or commit messages if you prefer.

https://github.com/SegmentLinking/cmssw/tree/LST_and_LSTCore

VourMa commented 1 month ago

Here's a new branch with squashed commits. I can change the branch name and/or commit messages if you prefer.

https://github.com/SegmentLinking/cmssw/tree/LST_and_LSTCore

Seems good to me. Should we force-push those commits in this branch, so that we can keep track of the discussions that we have already started here?

ariostas commented 1 month ago

Should we force-push those commits in this branch, so that we can keep track of the discussions that we have already started here?

Yeah, sounds good. I wasn't sure about that so I made a separate one to be safe, but let's just stick with this branch for discussions.

ariostas commented 1 month ago

There are lots of changes after the force-push, but that's just because I re-synced with upstream master.

slava77 commented 1 month ago

There are lots of changes after the force-push, but that's just because I re-synced with upstream master.

the actual diffs in the https://github.com/SegmentLinking/cmssw/pull/30/files look OK.

ariostas commented 2 weeks ago

Should we close this PR now? It seems like it's no longer needed for discussions.

VourMa commented 2 weeks ago

Should we close this PR now? It seems like it's no longer needed for discussions.

I think that the point of this PR is to mirror the ones in CMSSW master, so that when the central one is merged, this is also merged automatically. At least that's what I had understood based on the mkFit model of development.

slava77 commented 2 weeks ago

I think that the point of this PR is to mirror the ones in CMSSW master, so that when the central one is merged, this is also merged automatically. At least that's what I had understood based on the mkFit model of development.

yes, that's the idea. In addition, it's a convenient place to have aside discussion about changes there.

Also, being able to run our CI here would also be helpful; although the comparisons would have to be with an earlier version of itself; so, perhaps more complicated. Would it be possible to at least run the linter part?

VourMa commented 2 days ago

Should I sync the SegmentLinking/cmssw master to the cms-sw/cmssw master? I guess that would allow us to see the conflicts also here?

VourMa commented 2 days ago

Just for our internal coordination, I would like to mention that I am planning to work on the comments by Matt, implementing the majority of the cases, which mostly means removing magic numbers from our code.

slava77 commented 2 days ago

Should I sync the SegmentLinking/cmssw master to the cms-sw/cmssw master? I guess that would allow us to see the conflicts also here?

sure