Conversation
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for branch main. @smuzaffar, @Dr15Jones, @makortel, @aandvalenzuela, @iarspider, @cmsbuild can you please review it and eventually sign? Thanks. |
|
please test (The PR test for cms-sw/cmssw#41834 is really the one that tests this PR, the tests started by this comment don't mean much. I'm just running them to get the box checked...) |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cdcf12/32911/summary.html Comparison SummarySummary:
|
There was a problem hiding this comment.
The idea in cms-sw/framework-team#530 was to have the class versions encoded in the file name. That would help in the case a scouting class gets updated twice (with different PRs) between two adjacent (pre-)releases to make sure the correct file is kept and used in the tests (if the "latest file" has not been used in production, it would be deleted). In this case the file name would be then test3Run3Scouting_v3_v5_v3_v4_v5_v3_v5_v3_v3_CMSSW_12_4_0.root (assuming I extracted the class versions in their alphabetical order correctly). I admit managing the file versions is a bit cumbersome, but I hope it would still be beneficial in the long run. What do you think?
There was a problem hiding this comment.
I'll change the filenames.
It seemed kind of long when I was first looking at including 8 version numbers. It helps if we assume alphabetical order. Originally, I was thinking of how to identify the class associated with each version also and it was even longer. Currently I am making the data files after the release is already finalized. At least for these, the release number is associated with an exact set of versions. But if we want consistency going forward and there might be multiple versions in master while preparing a specific release, then it makes sense to include the version numbers.
There was a problem hiding this comment.
It seemed kind of long when I was first looking at including 8 version numbers. It helps if we assume alphabetical order. Originally, I was thinking of how to identify the class associated with each version also and it was even longer.
The alphabetical order was the least bad compromise I could come up with (between clarity and compactness). Could you include the convention in the README.md of the CMSSW PR?
There was a problem hiding this comment.
I just noticed this comment, but I think I explained the convention in the README already (the one I just pushed).
|
Pull request #1 was updated. |
|
please test files renamed, content unchanged |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cdcf12/32942/summary.html Comparison SummarySummary:
|
|
+1 |
|
merge |
4 participants
Add initial data files that will be used in a unit test of the Run 3 Scouting data formats. These will be read by future releases to verify backward compatibility.
Note the real test for this is in the unit test added to CMSSW. I will reference this PR from that PR (it will be submitted soon). After they are both approved, it would probably be better for this data PR to be merged first to avoid potential problems with the data files not being included in the CMSSW IB used for IB or PR tests. Then maybe wait a couple days to merge the CMSSW PR.