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

Use another PR in baseline for comparison tests #1188

Open
christopheralanwest opened this issue Aug 16, 2019 · 8 comments
Open

Use another PR in baseline for comparison tests #1188

christopheralanwest opened this issue Aug 16, 2019 · 8 comments

Comments

@christopheralanwest
Copy link
Contributor

christopheralanwest commented Aug 16, 2019

During the development of conditions, AlCa finds it useful to separate PRs that update the global tags in autoCond into logically separate pull requests. In particular, coupled code+conditions updates should not introduce additional, unrelated condition changes. This helps to cross-check that no changes were introduced into workflows other than those intended. For example:

The comparison tests currently make comparisons against the most recent IB but global tags are created and approved outside the CMSSW release framework. So, for example, the GTs in cms-sw/cmssw#27644 are final and all future global tags will include these updates. Where there are conflicts between the global tags in open PRs, global tags in the PRs that are intended to be merged later (to maintain consistency with the global tag versioning) are constructed relative to the global tags in autoCond (rather than relative to the global tag queues) purely for the purpose of supporting the comparison tests.

It would be better if we could use the correct global tag, based on the content of the global tag queue, in the PR from the beginning. To support the PR tests in this case, a new syntax would need to be introduced to the bot to use additional PRs in the baseline for the comparison tests. For example:

[@cmsbuild,] please test baseline <#PR[,#PR[...]]>

Then cms-sw/cmssw#27698 would be tested with

please test baseline 27644

and cms-sw/cmssw#27644 would be tested with

please test baseline 27698,27651

where cms-sw/cmssw#27698 would also include the updated global tag content of cms-sw/cmssw#27644.

@cmsbuild
Copy link
Contributor

A new Issue was created by @christopheralanwest .

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@kpedro88
Copy link
Contributor

Related to #937

It seems like we need a general scheme to generate, store, and track alternative comparison baselines.

@smuzaffar
Copy link
Contributor

smuzaffar commented Aug 29, 2019

@christopheralanwest , we have the results for PRs and I think with little modification we can use that as baseline. So if I understand correctly you will be doing

Note that in this case one can only start the tests once a baseline of other PR is already available. Also you can not say e.g. for cms-sw/cmssw#27644 please test baseline #27698 as baseline for only #27698 was never generated and uploaded.

@christopheralanwest
Copy link
Contributor Author

@smuzaffar What you propose isn't actually what I intended nor would it work for my use case involving autoCond updates. For the UL preparation, it is desirable to include finalized conditions in autoCond in master so that most of the updates are included in 11_0_X relval tests prior to their inclusion in 10_6_X. At the same time, there are PRs that include coupled code+conditions updates, and these should be tested separately from other condition updates. All of these PRs update the same lines in autoCond and so cannot be merged together. As backports are not merged as promptly as those in master, there may be three or four open PRs that modify the same line in autoCond.

In the scheme that I had in mind, your scheme would be written as, for example,

  • please test baseline #27651 with #27651
  • please test baseline #27651,#27698 with #27651,#27698

but that would not be sensible in the case of autoCond updates, in which global tags generally include all previous updates.

On another point: in your scheme one has to wait until the PR tests for the "earlier" PR(s) are completed prior to running the tests for the "later" PR. Since the tests take a non-negligible time to complete relative to the frequency of IB builds, the bot may use a different IB for the tests of the later PR. Could you instead ensure that when an alternate baseline is selected, the tests consistently use the same IB build?

@smuzaffar
Copy link
Contributor

@christopheralanwest , I am going to update bot which should allow us to generate baseline on demand (#1728 ). I think we can make use of this new feature to generate on demand baseline using extra PRs too e.g.

  • please test using baseline baseline-prs
  • please test with prs using baseline baseline-prs

can generate baseline using IB+baseline-prs and use it to compare the results for IB+pr or IB+pr+prs . This also ensure that both baseline and PR tests use the same IB.

Question to you, do we still need this feature and will the above implementation is enough to cover your use case?

@christopheralanwest
Copy link
Contributor Author

The feature requested in this PR would have been useful during the preparation for the Legacy production when new GT updates needed to be integrated before the PR including the previous set of updates had been merged. I don't know if GT changes are requested frequently enough nowadays that the feature is necessary. I'm no longer working on AlCa PRs so I don't need this feature myself, but I've added @cms-sw/alca-l2 to see if they would find it useful.

@francescobrivio
Copy link
Contributor

assign alca

  • we'll need to digest a bit this before giving a coheren answer :)

@cmsbuild
Copy link
Contributor

New categories assigned: alca

@yuanchao,@francescobrivio,@malbouis,@tvami you have been requested to review this Pull request/Issue and eventually sign? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants