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

Add warning if WF name will be too long for submission #29621

Merged
merged 2 commits into from
May 5, 2020

Conversation

srimanob
Copy link
Contributor

@srimanob srimanob commented May 3, 2020

PR description:

The character length of computing workflow is limited to 100 characters. The workflow name composes of 3 main parts:

  • username
  • RequestString
  • data/time of submission; This part is added during the submission, max. characters are 19, e.g. _200425_064221_5181.

This PR is to check if len(username_RequestString) is longer than 81 characters or not, then print the warning at the end when --wm init is used.

The goal is not to break the --wm init process, as one may need to just dump config for testing using any release (including IB). This PR just provides the warning in case someone would like to inject workflows later. Without this test, the injection will break when --wm force is used. However, some of the workflows will be submitted if they come before problematic workflows.

PR validation:

Test with 11_1_0_pre6:
runTheMatrix.py --what standard -l 136.886,136.7611,136.756 -t 8 -b 'DQM8CoresData' --label 'DQM8CoresData1234567890' --wm init

*** FIX NEEDED BEFORE INJECTION: 2 candidate workflows have too long name (>81 characters) ***
srimanob_RVCMSSW_11_1_0_pre6RunJetHT2016E_reminiaod__DQM8CoresData1234567890_RelVal_2016Ermaod (94 characters) 
srimanob_RVCMSSW_11_1_0_pre6RunZeroBias2016D__DQM8CoresData1234567890_RelVal_2016D (82 characters) 

workflow 136.866 is OK in this test,

srimanob_RVCMSSW_11_1_0_pre6RunEGamma2018D__DQM8CoresData1234567890_RelVal_2018D (80 characters long)

During injection, 136.866 will pass the injection, then break when 136.7611 injection is done.

if this PR is a backport please specify the original PR and why you need to backport that PR:

No need to backport.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29621/14971

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2020

A new Pull Request was created by @srimanob (Phat Srimanobhas) for master.

It involves the following packages:

Configuration/PyReleaseValidation

@chayanit, @cmsbuild, @wajidalikhan, @pgunnell, @kpedro88 can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @fabiocos, @slomeo this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@chayanit
Copy link

chayanit commented May 4, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5965/console Started: 2020/05/04 07:52

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

+1
Tested at: fc805c6
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bc6d8d/5965/summary.html
CMSSW: CMSSW_11_1_X_2020-05-03-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bc6d8d/5965/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2696239
  • DQMHistoTests: Total failures: 48
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2695872
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

Configuration/PyReleaseValidation/python/MatrixInjector.py Outdated Show resolved Hide resolved
Configuration/PyReleaseValidation/python/MatrixInjector.py Outdated Show resolved Hide resolved
Configuration/PyReleaseValidation/python/MatrixInjector.py Outdated Show resolved Hide resolved
Configuration/PyReleaseValidation/python/MatrixInjector.py Outdated Show resolved Hide resolved


if self.testMode and self.countLongWFName>0:
print("\n*** FIX NEEDED BEFORE INJECTION: "+str(self.countLongWFName)+" candidate workflows have too long name (>81 characters) ***")
Copy link
Contributor

Choose a reason for hiding this comment

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

raise an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix the text. This is for warning only that the workflows can't be submitted later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a situation where we want to allow those long workflow names? If not, I still think an exception would be better. That way, any PR creating an unnecessarily long workflow name must be fixed.

Copy link
Contributor Author

@srimanob srimanob May 4, 2020

Choose a reason for hiding this comment

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

As mentioned in the PR description, if someone would like to use "--wm init" to dump the configuration to run local-test only, we should not break it. This warning is targeted for people (like PdmV) who will submit relvals later, using "--wm submit, force" after "--wm init".

Configuration/PyReleaseValidation/python/MatrixInjector.py Outdated Show resolved Hide resolved
@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

Pull request #29621 was updated. @chayanit, @cmsbuild, @wajidalikhan, @pgunnell, @kpedro88 can you please check and sign again.

@chayanit
Copy link

chayanit commented May 4, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5976/console Started: 2020/05/04 17:38

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

+1
Tested at: 0236929
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bc6d8d/5976/summary.html
CMSSW: CMSSW_11_1_X_2020-05-04-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bc6d8d/5976/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2696239
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2695919
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@silviodonato
Copy link
Contributor

@kpedro88 does the last version of the code address your comments?
@chayanit @pgunnell @wajidalikhan do you have any comments?

@chayanit
Copy link

chayanit commented May 5, 2020

+1

@kpedro88
Copy link
Contributor

kpedro88 commented May 5, 2020

+upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2020

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

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

Successfully merging this pull request may close these issues.

5 participants