Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-7337

ChoiceField requires unicode strings

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pex_config
    • Labels:
      None

      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.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness 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
            tjenness 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
            rowen 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
            rowen 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
            tjenness 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
            tjenness 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
            rowen 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
            rowen 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
            tjenness 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
            tjenness 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
            rowen 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
            rowen 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
            tjenness Tim Jenness added a comment -

            Merged.

            Show
            tjenness Tim Jenness added a comment - Merged.

              People

              • Assignee:
                tjenness Tim Jenness
                Reporter:
                rowen Russell Owen
                Reviewers:
                Russell Owen
                Watchers:
                Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel