ChoiceField requires unicode strings

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.

blocks

Checklist

Lucidchart Diagrams

Issue Matrix

hide

Activity

Show:
Tim Jenness
August 23, 2016 at 2:45 PM

Merged.

Russell Owen
August 23, 2016 at 2:39 PM

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.

Tim Jenness
August 23, 2016 at 7:56 AM

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.

Russell Owen
August 23, 2016 at 7:51 AM

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.

Tim Jenness
August 23, 2016 at 7:44 AM

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.

Done
Pinned fields
Click on the next to a field label to start pinning.

Details

Assignee

Reporter

Reviewers

Russell Owen

Story Points

RubinTeam

Components

Checklist

Created August 22, 2016 at 6:00 PM
Updated September 1, 2016 at 1:53 PM
Resolved August 23, 2016 at 2:45 PM