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

Take throttled code from WMCore, see dmwm/WMCore#9158 #5880

Merged
merged 1 commit into from
May 21, 2019

Conversation

vkuznet
Copy link
Contributor

@vkuznet vkuznet commented Apr 23, 2019

Upon discussion in dmwm/DBS#600 I migrated Throttled code from CRABServer into WMCore. This PR removes throttled code and take it from WMCore.

@cmsdmwmbot
Copy link

Jenkins results:

  • Unit tests: succeeded
  • Pylint check: failed
    • 59 warnings and errors that must be fixed
    • 12 warnings
    • 97 comments to review
  • Pycodestyle check: succeeded
    • 1 comments to review
  • Python3 compatibility checks: failed
    • fails python3 compatibility test

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

@belforte
Copy link
Member

belforte commented Apr 23, 2019

I'll merge once there's a tagged version of WMCore with the change which we can build with.
I am keeping an eye on: dmwm/WMCore#9158

@vkuznet
Copy link
Contributor Author

vkuznet commented Apr 30, 2019

@belforte , Stefano, the WMCore throlling PR was merged by Alan, see dmwm/WMCore#9158 So feel free to check/test this PR and merge in CRAB code when it is convenient for you.

@belforte
Copy link
Member

This is in https://github.com/dmwm/WMCore/releases/tag/1.2.2
Let's merge and start building with that.

@belforte belforte merged commit 877dc88 into dmwm:master May 21, 2019
@belforte
Copy link
Member

@vkuznet I noticed that Pylint now reports a Python3 compatibility problem.
The report from this PR is not available in Jenkins anymore (maybe because I merge it, or because it was old), following text is from another PR, but this is your change. I am not sure why it only suggests to change this import, maybe to avoid clash with some Utils package in the standard distribution ? What do you suggest to do ? Should we look into a more unambiguous name from Utils package in WMCore ? Or just keep the complain ?

I have a simlar from Utils import blah in another file, and pylint does not complain about that apparetnly. Odd.


Summary of python3 compatibility report
Pre-python 2.6 constructs are introduced by this pull request. This must be fixed. Suggested patch follows:

--- ./src/python/CRABInterface/HTCondorDataWorkflow.py	(original)
+++ ./src/python/CRABInterface/HTCondorDataWorkflow.py	(refactored)
@@ -1,3 +1,4 @@
+from __future__ import absolute_import
 import re
 import json
 import time
@@ -20,7 +21,7 @@
 from WMCore.REST.Error import ExecutionError, InvalidParameter
 
 # WMCore Utils module
-from Utils.Throttled import global_user_throttle
+from .Utils.Throttled import global_user_throttle
 
 from CRABInterface.Utils import conn_handler
 from ServerUtilities import FEEDBACKMAIL, PUBLICATIONDB_STATES, isCouchDBURL, getEpochFromDBTime
 

@vkuznet
Copy link
Contributor Author

vkuznet commented May 21, 2019

Stefano, it seems to me that Throttling code never worked. Did you ever seen messages from it? I revisited the logic and it seems that the throttline limit was never reached. Therefore I took a liberty to re-factor the throttling code, see dmwm/WMCore#9211

We can address your issue in new PR once jenkins test will be available.

@belforte
Copy link
Member

I have never seen messages, but never expected Crab Server to be in need of throttling either. Anyhow this PR was about taking the code from WMCore and I am doing it now. Will build with current WMCore now, and with new tag with refactored code when available. Indeed python3 compatibility is more a curiosity than anything else atm.

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.

3 participants