search

spacetelescope / pandokia

Regression tests framework.
Other
2 stars 4 forks source link

Fixing python 3 deprecation warnings in our code base #64

Closed oiintam closed 4 years ago

oiintam commented 4 years ago

This PR includes any deprecation warning fixes:

Other details in sister PR: https://github.com/spacetelescope/pandeia/pull/4923

oiintam commented 4 years ago

Right, part of the reasons I want to put up this PR now is that I want to discuss how you guys would like to test this. Or should I just skip fixing Pandokia. (Actually only 7 out of 9 files are actually fixes, the other two files are docstring fixes.)

vglaidler commented 4 years ago

These look like fairly straightforward fixes. How will you test them?

oiintam commented 4 years ago

These look like fairly straightforward fixes. How will you test them?

I was wondering about that too. I have tested the lines I change with some sample inputs to see if they produce the same output. But that's all, it is hard to cover all the cases since I don't really know what kind of inputs actually pass in from the application that uses pandokia. For Pandeia, I did regression test and some simple testing on my test server.

vglaidler commented 4 years ago

In terms of testing, it was always an embarrasment to Mark and me that we did not have good tests for our testing software :smirk: but it was actually a tricky problem!

I think the best we can do is to make sure all our regular Pandokia operations still work with the changes, both for JWST and HST.

We could go through the effort to set things up to test before merge, but ISTM that would be pretty complicated. We could also merge and be sure to check tomorrow morning's test results, and report back to this PR with confirmation.

oiintam commented 4 years ago

So, is it okay for me to merge this now? And check tomorrow's test results?

vglaidler commented 4 years ago

Yes, merge now & check tomorrow @oiintam

ariedel commented 4 years ago

This broke pandokia on Python 2.7, which has no http module.

vglaidler commented 4 years ago

This broke pandokia on Python 2.7, which has no http module.

So do we want another PR that makes that import line depend on python version? It will still give us deprecation warnings in Py2 but Py2 itself is long past deprecated so 🤷

ariedel commented 4 years ago

We might want to do this for the Py3 performance testing; the alternative is making a branch from before this change every time we want to make a branch to compare py2 performance.

vglaidler commented 4 years ago

@oiintam what do you think?

oiintam commented 4 years ago

Adric, you said no http module, did you mean html?

Hmm, how often are we going to do python 2 testing? If you guys think we may need to do that multiple times in the future, then yes I can add it back. But if we only need it this time and this python 2 test will be our base line, then we just need the python 3 version since Adric already have his branch working. (but sorry for the hassle I put you in, Adric.)

vglaidler commented 4 years ago

I think the concern remains that we might yet need to do another patch release, if load continues to increase as the deadline approaches and we find other problems that need to be fixed.

But we could also decide that it's not worth the effort to do "in case" and remember that we need to do it if we do have another Py2 patch.

OTOH it is not very much effort, might be easier todo than to remember.

oiintam commented 4 years ago

Okay, if there is a value and possibility to use the functionality, then it is a strong enough argument to convince me. I will make a branch to change this and make a PR.

ariedel commented 4 years ago

Yes, I meant html

ariedel commented 4 years ago

My personal plans to do performance comparisons would involve quite a few tests, though only on one or two branches. I don't know what @cdsontag is expecting to do...

cdsontag commented 4 years ago

I can only imagine a need (myself) to drill down, for perf. checking reasons, a few times on py2, if that. I imagine I can do some overall simple tests on production itself (no code changes), using wall-clock time, and then compare to fully timed runs on py3.

So that's my way of saying I don't see a need of a lot of py2 testing beyond our side-by-side test. So I don't see a need to do anything heroic to get pdk to run in py2. Whatever you all want to do.