Adds WMCMSSWSubprocess metrics to FJR document #11716

vkuznet · 2023-09-13T18:22:24Z

Fixes #11660
Complement to #11665

Status

ready

Description

Add WMCMSSWSubproces metrics to WMCore FJR stored in WMAgent local CouchDB. Full procedure is described in the following wiki.

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

YES

Related PRs

#11692

External dependencies / deployment changes

cmsdmwmbot · 2023-09-13T18:36:12Z

Jenkins results:

Python3 Unit tests: failed
- 3 new failures
- 1 tests no longer failing
- 1 changes in unstable tests
Python3 Pylint check: failed
- 4 warnings and errors that must be fixed
- 3 warnings
- 123 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 6 comments to review

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

amaltaro

@vkuznet in addition to the comments along the code, can you please provide unit test(s) for these changes? Thanks

amaltaro · 2023-09-14T09:47:18Z

src/python/WMCore/Services/WMArchive/DataMap.py

@@ -113,7 +112,6 @@
        # 'StageOutCommand': ''
    }],
    'WMCMSSWSubprocess': {},
-    'WMTiming': {},


Are you removing this because this isn't yet fully functional? If so, then don't you prefer to leave it in while we work on getting this WMTiming metric in the proper fwjr document?

I am removing this because it does not belong to this step part of the report and it should be at top level. But so far I do not see any defaults for top level parts in this codebase, and only defaults for step parts.

amaltaro · 2023-09-14T09:49:11Z

src/python/WMCore/FwkJobReport/Report.py

@@ -1410,6 +1424,17 @@ def getPrepID(self):
        """
        return getattr(self.data, 'prep_id', "")

+    def getWMTiming(self):
+        """
+         _getWMTiming_


Please refresh this contribution guidelines: https://github.com/dmwm/WMCore/blob/master/CONTRIBUTING.rst#project-docstrings-best-practices

It seems you also have trailing spaces here.

will do, I copied docstring as it is used in other parts in this code. And, new docstring style will differ from other methods just saying.

That's fine. At least we will be slowly converging towards one style.

amaltaro · 2023-09-14T09:49:48Z

src/python/WMCore/FwkJobReport/Report.py

@@ -301,6 +303,18 @@ def getExitCodes(self):
            returnCodes.update(self.getStepExitCodes(stepName=stepName))
        return returnCodes

+    def getWMCMSSWSubprocess(self, stepName):
+        """
+        _getWMCMSSWSubprocess_


Same comment as below, please see https://github.com/dmwm/WMCore/blob/master/CONTRIBUTING.rst#project-docstrings-best-practices

cmsdmwmbot · 2023-09-14T11:59:50Z

Jenkins results:

Python3 Unit tests: succeeded
- 1 changes in unstable tests
Python3 Pylint check: failed
- 4 warnings and errors that must be fixed
- 3 warnings
- 123 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 6 comments to review

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

amaltaro

@vkuznet Valentin, this code looks good to me, despite a few minor comments that have not been addressed from the previous review.

In addition, I would suggest to:

refactor the initial PR description (IMO, most of that content belongs to the wiki that you created, for the data flow of CMSSW metrics).
if you are done working on the PR, please remove that label as well (PR: work in progress)
please provide unit tests for the 2 new getter methods.

amaltaro · 2023-09-18T20:05:11Z

src/python/WMCore/FwkJobReport/Report.py

@@ -301,6 +303,18 @@ def getExitCodes(self):
            returnCodes.update(self.getStepExitCodes(stepName=stepName))
        return returnCodes

+    def getWMCMSSWSubprocess(self, stepName):
+        """
+        _getWMCMSSWSubprocess_


amaltaro · 2023-09-18T20:05:20Z

src/python/WMCore/FwkJobReport/Report.py

@@ -1410,6 +1424,17 @@ def getPrepID(self):
        """
        return getattr(self.data, 'prep_id', "")

+    def getWMTiming(self):
+        """
+         _getWMTiming_


cmsdmwmbot · 2023-09-18T23:22:14Z

Jenkins results:

Python3 Unit tests: failed
- 1 new failures
- 4 tests no longer failing
- 2 tests added
- 1 changes in unstable tests
Python3 Pylint check: failed
- 4 warnings and errors that must be fixed
- 3 warnings
- 178 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 25 comments to review

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

vkuznet · 2023-09-19T12:09:27Z

@amaltaro , I updated description of this PR, removed label and added unit tests.
Regarding your requests:

I never created wiki about it but rather had google doc. Therefore, to fulfill your request I created new wiki. It may still require adjustments though when I'll resolve WMTiming metrics and generalize it
the old style _getWMTiming_ docstring are removed, it was not clear to me if you wanted them to keep or not, but I added proper documentation to method
unit test are created and according to test they passed, but it looks like there is one unstable unit test (Throttling) which is not related to this PR.

Please review this PR again and let me know if it is ok.

amaltaro

@vkuznet changes look good to me, but first please squash these commits accordingly and if needed, amend the commit message as well.

vkuznet · 2023-09-19T13:55:31Z

Squash is done

cmsdmwmbot · 2023-09-19T14:05:17Z

Jenkins results:

Python3 Unit tests: failed
- 1 new failures
- 2 tests added
- 1 changes in unstable tests
Python3 Pylint check: failed
- 4 warnings and errors that must be fixed
- 3 warnings
- 178 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 25 comments to review

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

amaltaro · 2023-09-19T14:06:46Z

Thanks, Valentin.

vkuznet added the PR: Work in progress label Sep 13, 2023

vkuznet self-assigned this Sep 13, 2023

vkuznet requested a review from amaltaro September 13, 2023 18:22

amaltaro requested changes Sep 14, 2023

View reviewed changes

amaltaro mentioned this pull request Sep 14, 2023

WM measurements of wallclock and cpu time for each cmsRun step #11660

Closed

vkuznet requested a review from amaltaro September 18, 2023 13:10

amaltaro requested changes Sep 18, 2023

View reviewed changes

vkuznet removed the PR: Work in progress label Sep 19, 2023

vkuznet requested a review from amaltaro September 19, 2023 12:16

amaltaro added the PR: Update documentation label Sep 19, 2023

amaltaro approved these changes Sep 19, 2023

View reviewed changes

vkuznet added 2 commits September 19, 2023 09:54

Adds WMCMSSWSubprocess metrics to FJR document

10a4675

Add new unit tests for WMTiming and WMCMSSWSubprocess

4a12754

vkuznet force-pushed the fix-wma-integration branch from dab6c6d to 4a12754 Compare September 19, 2023 13:55

amaltaro merged commit 27a5dec into dmwm:master Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds WMCMSSWSubprocess metrics to FJR document #11716

Adds WMCMSSWSubprocess metrics to FJR document #11716

vkuznet commented Sep 13, 2023 •

edited

Loading

cmsdmwmbot commented Sep 13, 2023

amaltaro left a comment

amaltaro Sep 14, 2023

vkuznet Sep 14, 2023

amaltaro Sep 14, 2023

vkuznet Sep 14, 2023

amaltaro Sep 14, 2023

amaltaro Sep 18, 2023

amaltaro Sep 14, 2023

amaltaro Sep 18, 2023

cmsdmwmbot commented Sep 14, 2023

amaltaro left a comment

amaltaro Sep 18, 2023

amaltaro Sep 18, 2023

cmsdmwmbot commented Sep 18, 2023

vkuznet commented Sep 19, 2023

amaltaro left a comment

vkuznet commented Sep 19, 2023

cmsdmwmbot commented Sep 19, 2023

amaltaro commented Sep 19, 2023

Adds WMCMSSWSubprocess metrics to FJR document #11716

Adds WMCMSSWSubprocess metrics to FJR document #11716

Conversation

vkuznet commented Sep 13, 2023 • edited Loading

Status

Description

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

Related PRs

External dependencies / deployment changes

cmsdmwmbot commented Sep 13, 2023

amaltaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsdmwmbot commented Sep 14, 2023

amaltaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsdmwmbot commented Sep 18, 2023

vkuznet commented Sep 19, 2023

amaltaro left a comment

Choose a reason for hiding this comment

vkuznet commented Sep 19, 2023

cmsdmwmbot commented Sep 19, 2023

amaltaro commented Sep 19, 2023

vkuznet commented Sep 13, 2023 •

edited

Loading