ChoiceField requires unicode strings

XMLWordPrintable

Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
0.2
• Team:
Architecture

Description

lsst.pex.config.ChoiceField fails in Python 2 with:

 ValueError: ChoiceField's allowed choice variance is of incorrect type str. Expected future.types.newstr.newstr 

if created using normal strings for the choices or default. I saw this in noiseReplacer.py when converting meas_base to be Python 3 compatible.

Activity

Hide
Tim Jenness added a comment -

Russell Owen I've done a quick fix for the problem. It won't fix all possible combinations of unicode, string literals and future str but let me know if it fixes your particular problem.

Show
Tim Jenness added a comment - Russell Owen I've done a quick fix for the problem. It won't fix all possible combinations of unicode, string literals and future str but let me know if it fixes your particular problem.
Hide
Russell Owen added a comment -

That fixed the problem in meas_base. Do you recommend that I undo my workaround in meas_base and make DM-7262 depend on this ticket?

Show
Russell Owen added a comment - That fixed the problem in meas_base. Do you recommend that I undo my workaround in meas_base and make DM-7262 depend on this ticket?
Hide
Tim Jenness added a comment -

Actually, I'm surprised it works with your workaround in place given the unicode strings being passed in. I'd prefer it if we backed out the patch as it should work without it now.

Show
Tim Jenness added a comment - Actually, I'm surprised it works with your workaround in place given the unicode strings being passed in. I'd prefer it if we backed out the patch as it should work without it now.
Hide
Russell Owen added a comment -

Interesting. I had not even tried your fix to pex_config against my patched meas_base code. I just did and it fails, as you predicted. That's a bit disturbing, but in any case I'll undo my workaround in meas_base and make that ticket depend on this one.

Show
Russell Owen added a comment - Interesting. I had not even tried your fix to pex_config against my patched meas_base code. I just did and it fails, as you predicted. That's a bit disturbing, but in any case I'll undo my workaround in meas_base and make that ticket depend on this one.
Hide
Tim Jenness added a comment -

Right, pex.config.Config has an autocast function in it that converts str to classic str so that won't be a problem. Everything works on python 3 because there is only one str. If we code like we always have in the past (no unicode) then we'll never trigger the problem. If people start throwing unicode around then we'll have to revisit pex_config. The problem is that it tests that the data type you said you were using matches the data type of the choices you supply. Future str is actually unicode so you fixed it by giving unicode choices. All the code we have is assuming str is the same type as string literal so I made that true in pex_config. In theory I could add an "expected failure" test for python2 but I don't think it gains us anything. A more complete fix of pex_config to properly handle string types will require a bit of thought.

Show
Tim Jenness added a comment - Right, pex.config.Config has an autocast function in it that converts str to classic str so that won't be a problem. Everything works on python 3 because there is only one str . If we code like we always have in the past (no unicode) then we'll never trigger the problem. If people start throwing unicode around then we'll have to revisit pex_config. The problem is that it tests that the data type you said you were using matches the data type of the choices you supply. Future str is actually unicode so you fixed it by giving unicode choices. All the code we have is assuming str is the same type as string literal so I made that true in pex_config . In theory I could add an "expected failure" test for python2 but I don't think it gains us anything. A more complete fix of pex_config to properly handle string types will require a bit of thought.
Hide
Russell Owen added a comment -

This looks good to me. I verified that it is possible to override Choice entries from the command line (by tweaking pipe_base examples/argumentParser.py to add a choice field) and that the new unit test fails with the old code. The test could be expanded to make a Config and run validate and such on it, but given that the test fails with the old code, it probably suffices.

Show
Russell Owen added a comment - This looks good to me. I verified that it is possible to override Choice entries from the command line (by tweaking pipe_base examples/argumentParser.py to add a choice field) and that the new unit test fails with the old code. The test could be expanded to make a Config and run validate and such on it, but given that the test fails with the old code, it probably suffices.
Hide
Tim Jenness added a comment -

Merged.

Show
Tim Jenness added a comment - Merged.

People

Assignee:
Tim Jenness
Reporter:
Russell Owen
Reviewers:
Russell Owen
Watchers:
Russell Owen, Tim Jenness