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

Skip alert for RelVals with deletable pileup #11283

Merged
merged 2 commits into from
Sep 14, 2022
Merged

Conversation

amaltaro
Copy link
Contributor

Fixes #11279

Status

not-tested

Description

Change was supposed to be very basic as of not creating alert notifications for Release Validation workflows. However, we cannot distinguish RelVals from non-RelVals in MSRuleCleaner, hence the code refactoring avoiding to duplicate code.

Note that this PR adds a new SubRequestType key/value to the MSRuleCleanerWflow data structure.

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

None

External dependencies / deployment changes

None

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 6 new failures
    • 1 tests deleted
    • 1 tests added
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 236 warnings and errors that must be fixed
    • 16 warnings
    • 197 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 31 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13579/artifact/artifacts/PullRequestReport.html

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 5 new failures
    • 1 tests deleted
    • 1 tests added
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 236 warnings and errors that must be fixed
    • 16 warnings
    • 197 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 31 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13580/artifact/artifacts/PullRequestReport.html

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 4 new failures
    • 1 tests deleted
    • 1 tests added
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 571 warnings and errors that must be fixed
    • 16 warnings
    • 207 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 31 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13581/artifact/artifacts/PullRequestReport.html

@amaltaro
Copy link
Contributor Author

test this please

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 1 tests deleted
    • 1 tests added
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 571 warnings and errors that must be fixed
    • 16 warnings
    • 207 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 31 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13582/artifact/artifacts/PullRequestReport.html

Fix Workflow data structure to use isRelVal function

update MSOutputTemplate to use the common function

fix docstring
fix more unit tests
@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 1 tests deleted
    • 1 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: failed
    • 571 warnings and errors that must be fixed
    • 16 warnings
    • 207 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 31 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13583/artifact/artifacts/PullRequestReport.html

Copy link
Contributor

@todor-ivanov todor-ivanov 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. Thanks @amaltaro

@@ -200,6 +201,7 @@ def docSchema(self):
docTemplate = [
('RequestName', None, (bytes, str)),
('RequestType', None, (bytes, str)),
('SubRequestType', None, (bytes, str)),
Copy link
Contributor

@vkuznet vkuznet Sep 14, 2022

Choose a reason for hiding this comment

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

Due to this change I want to raise your awareness of consequences of having multiple data-type assignments to a single variable. Why do you do that? Such assignment only creates other problems down in the road making checks, and code to do extra work and hide issue with data-types. All conversions should be done in place and attributes in a document should have single data type. But it is my view and I doubt you'll make the changes. But as reviewer I want to want you about it.

For instance, if this code produce data with different data-type (in a list one document will have bytes data-type in another document it will be a string), the issue will raise in a client code which can't process the data using a parser which relies on specific data-type. Especially if such code is statically typed language (C/C++, Go, etc.) In such languages parser can't assign different data type to a variable with certain data-type. As such such data will fail to parse in such languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Byte and unicode strings was very hard to deal while making WMCore compatible with py2 and py3. As we start removing python2 code and making WMCore python3-only, we should definitely revisit strings and try to converge to a single type as much as we can in the project. Good point!

For now, I am just following the stream here.

@amaltaro amaltaro merged commit 8f0b9ea into dmwm:master Sep 14, 2022
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.

Do not create notifications in MSRuleCleaner for RelVal pileup
4 participants