Drop python 2 idioms and compatibility fixes #1567
Conversation
pyupgrade --py36-plus $(find . -name "*.py") --keep-mock
com2ann .
fix_logger_format.py --do
Using local branch fixes for: - mixed mapping/plain printf types - Applying %d refactoring for int/len calls instead of just agressive mode - Prevent partial conversions when some specifiers not understood - Handle '-' and ' ' conversion flags flynt .
|
Change set looks good to me. Assuming there are no objections I would be tempted to merge this ~ ASAP and watch very carefully for any unexpected problems. Scanned the first ~ 20% quite carefully, next 40% very lightly, but assuming this is a bulk change (which it looks like, changes were duplicated) would be very keen on seeing this go in. |
Codecov Report
@@ Coverage Diff @@
## master #1567 +/- ##
==========================================
- Coverage 66.72% 66.55% -0.18%
==========================================
Files 616 614 -2
Lines 69290 68682 -608
Branches 9572 9562 -10
==========================================
- Hits 46234 45711 -523
+ Misses 21120 21044 -76
+ Partials 1936 1927 -9 |
|
Hi, here are the xfel_regression test instructions: Look under the General build instructions, then skip down below to the cctbx.xfel tests section. |
👍 |
No instruction for source for |
Found at https://gitlab.com/cctbx/ |
|
@phyy-nx on this now, though plenty of tests fail on current etc... |
|
This is all based on a fresh-as-a-daisy brand new |
|
Turns out I was running against Python 3.9 which is possibly a little too cutting edge - however with @ndevenish branch I get the same errors, which I read as a good thing Rebasing on 3.6 now - thanks for spotting that @Anthchirp |
|
On 3.6.12: I imagine that these were hidden behind the |
|
Once more, on the branch, ... -> the same so good to go, in my book |
|
uc_metrics is pulled in by the xfel builder. Did you use --builder=xfel or --builder=dials? I note the LCLS build instructions use the dials builder. I need to update those to the xfel builder too. The two failing merging tests are weird. The file was removed a while ago. Oh! I see what's happening here. The platform tag is not working for this test. Look at tst_xfel_merge's function get_platform_tag. What is this finding for your system? |
|
More detail, platform instability led us to create 3 versions of our reference datasets. These are the datasets the results of merging are compared to. So there are three files for these tests, labeled _Darwin, _C7 and _C6, for macos and two centos versions. We are dropping centos 6 support soon so this can likely be simplified a bit in the future. |
Ah, so what I did was build with the @phyy-nx assume you ran the tests yourself using the cctbx-type build as well right? Any difference? From what I can tell the changes in this PR make no difference to the outcomes. |
|
Nope, I haven't ran the xfel_regression tests on this PR. I was hoping they would work in your hands, since that would accomplish both testing this PR and getting xfel_regression used more generally :) That said, I think get_platform_tag just isn't working on your system. What system is it? |
RHEL7 on AMD EPYC |
|
Hi, can you run this snippet: I get: |
|
|
Guess, deprecation a long time ago has now come home to roost... |
|
Right, distro is the replacement for platform. I've been meaning to bring
this up as a new dependency.
Here's how to get it for a later python version:
conda install -c conda-forge distro
Then the snippet should work.
|
Assertion verified: |
|
I don't get it. That looks like it should resolve to the tag '_C7' Like this: Anyway this diff should make the tests run, if you don't mind checking again: |
|
OK, made the diffs you propose but still find: I respectfully assert that this has moved significantly off-topic from the "Python 2 idioms" into debugging your platform variant tests and weird test failures - which are identical with and without the branch being checked out. I feel that I have done (more than) due dilligence with running these tests (which I assume you still have not run yourself?) so I think I would like to see this merged. I don't mind too much helping you to debug these tests in a separate thread, but not here. I'm confident that the change set @ndevenish proposes is good and will save us from making these changes piecemeal in every subsequent commit we make. |
|
FWIW the default to OK, that is rather elementary: it cannot be imported as it is not there to import |
I can only assume that we do not have this -> this is the issue here. Am satisfied that the criteria merging this branch are met. |
There was a problem hiding this comment.
Reviewing the change set these look sane and reasonable - this will save us from making the changes piecemeal across the next (forever) commits so worthwhile. Extensive testing across DIALS and XFEL code bases indicates no change to the test outcomes -> the code is good, please merge.
|
Thanks for the due diligence |
This PR drops most of the python 2/3 compatibility layers, drops requirements for six/futures, and updates lots of idioms to their newer equivalents.
The filters applied, separated into different commits (so we can drop them if there is something we are unsure about), are:
__future__, new-style classes, simple fstrings, modernsuper(), removesix. (47134df).formatand%formatting inside logging calls to use the explicit logging lazy-format style. This means that they won't be auto-converted by flynt (38b6010).' 'and'-'prefixes, convertingintandlencalls, and being slightly more cautious around complicated format-strings. (22b34b5)sixand(6405369)futuremath.isclosebackport (a44f1ea)str()calls in converted f-strings, and some other minor format fixes. (65f3d41)installer/bootstrap.pyhas been excluded from all conversions.