[py2py3] More job runtime places to encode pickledarguments to bytes #10766
Conversation
|
Jenkins results:
|
There was a problem hiding this comment.
Hello! I have to trust your knowledge here, @amaltaro. From a quick look it seems that pickledarguments is only set as
self.data.application.configuration.pickledarguments = pickle.dumps(args, protocol=0)and pickle.dumps() return bytes. I have to assume that we may be loading this self.data.application.configuration from a different place (maybe directly from some file?) where pickledarguments ends up being a unicode string.
In any case, I like the approach of converting it back to right type as late as possible, when needed. Approved!
| pklArgs = encodeUnicodeToBytesConditional(stepSection.application.configuration.pickledarguments, | ||
| condition=PY3) | ||
| args = pickle.loads(pklArgs) |
There was a problem hiding this comment.
I like that you are trying to fix only what breaks, but pickle.loads() expects the data to be of type bytes both in py2 and py3, so we could avoid using the conditional here. but do not worry, we should not have problem!
There was a problem hiding this comment.
Yes, I wanted to be as conservative as possible. At least in central production, I can confirm that that attribute comes as bytes (since we migrated services to Py3).
Okay, I'm not going to update this patch then (otherwise I have to re-run my tests). But we better leave it as unresolved for future reference. Thanks!
|
And you are correct, Dario. We do load this from a file (in couchdb). If you look at this workflow from 2020: one can see this stringification: which is exactly what was breaking. |
|
Tests with Py3 WMAgent went well. |
Fixes #10647
Status
ready
Description
For workflows that have been created while ReqMgr2 was still running in Python2 (< July 2021), the CMSSW
pickledargumentscan be of string type, thus failing to be pickled loaded.This patch will encode those native strings to bytes, if running with a Python3 interpreter.
Is it backward compatible (if not, which system it affects?)
YES (it should!)
Related PRs
Complement to: #10648
External dependencies / deployment changes
None