# Port pipe_tasks to Python 3

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
3
• Sprint:
• Team:

#### Activity

Hide
Fred Moolekamp added a comment -

Lauren MacArthur I had to change the tolerance on the testProcessCcd tests in pipe_tasks. It appears that the previous restrictions were too tight but can you verify that the new values I've chosen fall within acceptable limits? See https://github.com/lsst/pipe_tasks/pull/77/commits/9daa645ad793354a20849348fef3720909006b34

Show
Fred Moolekamp added a comment - Lauren MacArthur I had to change the tolerance on the testProcessCcd tests in pipe_tasks. It appears that the previous restrictions were too tight but can you verify that the new values I've chosen fall within acceptable limits? See https://github.com/lsst/pipe_tasks/pull/77/commits/9daa645ad793354a20849348fef3720909006b34
Hide
Fred Moolekamp added a comment -

All of the tests pass with the current parameter values but per Tim Jenness and Russell Owen the setL method in afw is broken, which truncates visit ID's to 32 bits. This will need to be fixed before this ticket can be completed.

Show
Fred Moolekamp added a comment - All of the tests pass with the current parameter values but per Tim Jenness and Russell Owen the setL method in afw is broken, which truncates visit ID's to 32 bits. This will need to be fixed before this ticket can be completed.
Hide
Lauren MacArthur added a comment -

I didn't change the tolerances, just the values, so it's not obvious why the tolerances are now too stringent (and by so much!). Did the test pass on whatever compiler you're using in a pre-DM-6815 state? Is it possible your stack is not entirely in sync with master (I doubt it, but it's worth a double check)??

These tolerances were set here by Russell Owen, so he may have some opinion as to how tight they ought to be (although he does say in the commit message that the choice was arbitrary).

Show
Lauren MacArthur added a comment - I didn't change the tolerances, just the values, so it's not obvious why the tolerances are now too stringent (and by so much!). Did the test pass on whatever compiler you're using in a pre- DM-6815 state? Is it possible your stack is not entirely in sync with master (I doubt it, but it's worth a double check)?? These tolerances were set here by Russell Owen , so he may have some opinion as to how tight they ought to be (although he does say in the commit message that the choice was arbitrary).
Hide
Fred Moolekamp added a comment -

The difference is the python 3 vs python 2 stack. The python 3 stack was just rebuilt today, so it is definitely up to date, and I rebased my working version of pipe_tasks. Unless we believe this is a bug in the code running in python 3 the best thing to do might be to open up a ticket to review the requirements of the test and leave the current tolerances for the python 3 port. Tim Jenness, John Swinbank, does that sound reasonable?

Show
Fred Moolekamp added a comment - The difference is the python 3 vs python 2 stack. The python 3 stack was just rebuilt today, so it is definitely up to date, and I rebased my working version of pipe_tasks. Unless we believe this is a bug in the code running in python 3 the best thing to do might be to open up a ticket to review the requirements of the test and leave the current tolerances for the python 3 port. Tim Jenness , John Swinbank , does that sound reasonable?
Hide
John Swinbank added a comment -

Those changes look very substantial to me: I'm not comfortable with them without more explanation. Correct me if I'm wrong, but I don't think we've seen switching to Python 3 making a big change to our numerics elsewhere.

We do know we get numerical differences between Mac and Linux (although they're quite small). Have you checked this test on both platforms?

Show
John Swinbank added a comment - Those changes look very substantial to me: I'm not comfortable with them without more explanation. Correct me if I'm wrong, but I don't think we've seen switching to Python 3 making a big change to our numerics elsewhere. We do know we get numerical differences between Mac and Linux (although they're quite small). Have you checked this test on both platforms?
Hide
John Swinbank added a comment -

(After discussion with Simon Krughoff, setting this back to the AP team and letting Fred off the hook!)

Show
John Swinbank added a comment - (After discussion with Simon Krughoff , setting this back to the AP team and letting Fred off the hook!)
Hide
Meredith Rawls added a comment - - edited

Thanks to some sleuthing by John Parejko, we discovered that python's random number generator spits out different values in py2 and py3, which is likely behind this discrepancy. Running this simple code (from this site) yields different results in our py2 and py3.

 import random random.seed(0) for x in range(0, 10):  random.randint(0, 1000000) 

py2 result:
844422
757955
420572
258917
511275
404934
783799
303313
476597
583382

py3 result:
885440
403958
794772
933488
441001
42450
271493
536110
509532
424604

However, doing the same thing with from numpy import random gives the same set of values in both cases:
985772
305711
435829
117952
963395
152315
882371
359783
304137
122579

Show
Meredith Rawls added a comment - - edited Thanks to some sleuthing by John Parejko , we discovered that python's random number generator spits out different values in py2 and py3, which is likely behind this discrepancy. Running this simple code ( from this site ) yields different results in our py2 and py3. import random random.seed(0) for x in range(0, 10): random.randint(0, 1000000) py2 result: 844422 757955 420572 258917 511275 404934 783799 303313 476597 583382 py3 result: 885440 403958 794772 933488 441001 42450 271493 536110 509532 424604 However, doing the same thing with from numpy import random gives the same set of values in both cases: 985772 305711 435829 117952 963395 152315 882371 359783 304137 122579
Hide
Meredith Rawls added a comment -

Unfortunately, it appears that changing import random to from numpy import random in both measurePsf and imageDifference has no change on what testProcessCcd.py spits out in either python version. (I also took care to change random.sample() -> numpy.random.choice().)

Show
Meredith Rawls added a comment - Unfortunately, it appears that changing import random to from numpy import random in both measurePsf and imageDifference has no change on what testProcessCcd.py spits out in either python version. (I also took care to change random.sample() -> numpy.random.choice() .)
Hide
Meredith Rawls added a comment -

The aforementioned random was not the underlying issue here at all - the numerical differences are being caused by a difference in the background model. This is outside the scope of this ticket, so I'm going to wrap it up and open new tickets pertaining to meas_algorithms. A full writeup of what I found is here on Community. My only changes to the code wound up being formatting tweaks, including DM-7769.

Show
Meredith Rawls added a comment - The aforementioned random was not the underlying issue here at all - the numerical differences are being caused by a difference in the background model. This is outside the scope of this ticket, so I'm going to wrap it up and open new tickets pertaining to meas_algorithms . A full writeup of what I found is here on Community . My only changes to the code wound up being formatting tweaks, including DM-7769 .
Hide
Meredith Rawls added a comment - - edited

Your name came up as a possible reviewer for this, Nate Lust. Please let me know if you can't do it - thanks!

Show
Meredith Rawls added a comment - - edited Your name came up as a possible reviewer for this, Nate Lust . Please let me know if you can't do it - thanks!
Hide
Nate Lust added a comment -

I was away last week, and am just getting back to the swing of things. We have a planning meeting today, but I will get to this in the next day or two. Does this time frame work for you?

Show
Nate Lust added a comment - I was away last week, and am just getting back to the swing of things. We have a planning meeting today, but I will get to this in the next day or two. Does this time frame work for you?
Hide
Meredith Rawls added a comment -

That's fine. I think it should be a quick review. Thank you!

Show
Meredith Rawls added a comment - That's fine. I think it should be a quick review. Thank you!
Hide
Nate Lust added a comment -

Just a few minor points, but I think they might be addressed in a ticket you mentioned. Please make sure they are covered in work somewhere, and that this passes jenkins and is rebased up to current. Then you are good to merge.

Show
Nate Lust added a comment - Just a few minor points, but I think they might be addressed in a ticket you mentioned. Please make sure they are covered in work somewhere, and that this passes jenkins and is rebased up to current. Then you are good to merge.
Hide
Meredith Rawls added a comment -

Thanks, yes, I opened new tickets because there's a deeper py2/py3 background mismatch beyond the scope of this py3 port.

It turns out the py2 Jenkins is failing because of a string issue that I missed before. (I must have only checked it with py3 Jenkins.) I traced the problem to something in matchBackground that uses listField from pex_config. I'll let you know once I fix it so py2 Jenkins passes and ask you to please take a final look.

Show
Meredith Rawls added a comment - Thanks, yes, I opened new tickets because there's a deeper py2/py3 background mismatch beyond the scope of this py3 port. It turns out the py2 Jenkins is failing because of a string issue that I missed before. (I must have only checked it with py3 Jenkins.) I traced the problem to something in matchBackground that uses listField from pex_config . I'll let you know once I fix it so py2 Jenkins passes and ask you to please take a final look.
Hide
Meredith Rawls added a comment -

I've gotten pipe_tasks and pex_config to pass py2 Jenkins by making a tickets/DM-7292 branch for pex_config and editing pex_config/listField per Tim Jenness' suggestion. However, the full build I ran failed on obs_subaru. Something I didn't touch, pex_config/dictField, is apparently causing a FieldValidationError when obs_subaru is tested. I can't tell if this problem arose indirectly from my small edit to pex_config/listField or not? It's strings all the way down. Here is the relevant Jenkins brain dump. Input appreciated.

Show
Meredith Rawls added a comment - I've gotten pipe_tasks and pex_config to pass py2 Jenkins by making a tickets/ DM-7292 branch for pex_config and editing pex_config/listField per Tim Jenness ' suggestion. However, the full build I ran failed on obs_subaru . Something I didn't touch, pex_config/dictField , is apparently causing a FieldValidationError when obs_subaru is tested. I can't tell if this problem arose indirectly from my small edit to pex_config/listField or not? It's strings all the way down. Here is the relevant Jenkins brain dump. Input appreciated.
Hide
Tim Jenness added a comment -

My guess is that listField needs the same str translation fix.

Show
Tim Jenness added a comment - My guess is that listField needs the same str translation fix.
Hide
Meredith Rawls added a comment -

I did put your str translation fix in listField, but it looks to me like dictField is written differently and doesn't explicitly use {{dtype}}s.

Show
Meredith Rawls added a comment - I did put your str translation fix in listField , but it looks to me like dictField is written differently and doesn't explicitly use {{dtype}}s.
Hide
Meredith Rawls added a comment -

This went much deeper than it needed to... turns out there were still a couple from builtins import str lines in pipe_tasks that shouldn't have been there. They're gone now, and Jenkins is happy on py2 and py3! The tickets/DM-7292 branch of pex_config is identical to master and may be ignored. Nate Lust, please confirm you are OK with me going ahead and merging.

Show
Meredith Rawls added a comment - This went much deeper than it needed to... turns out there were still a couple from builtins import str lines in pipe_tasks that shouldn't have been there. They're gone now, and Jenkins is happy on py2 and py3! The tickets/ DM-7292 branch of pex_config is identical to master and may be ignored. Nate Lust , please confirm you are OK with me going ahead and merging.
Hide
Meredith Rawls added a comment -

Discussed with Nate in Slack, fixed up a couple last tiny things, all good!

Show
Meredith Rawls added a comment - Discussed with Nate in Slack, fixed up a couple last tiny things, all good!

#### People

Assignee:
Meredith Rawls
Reporter:
Tim Jenness
Reviewers:
Nate Lust
Watchers:
Fred Moolekamp, John Parejko, John Swinbank, Lauren MacArthur, Meredith Rawls, Nate Lust, Tim Jenness