dxtbx -> Python 3

[graeme-winter]

Python 3 for dxtbx

As you are aware dxtbx is already python 3 syntax compatible, and has been for some time. With cctbx now being nominally python 3 compatible we can proceed with our work on dxtbx.

As discussed on dials-support we propose that this is done in a staged way, with open work and continuous testing to record progress.

Process

Before the work is started we will set up a Jenkins build job at Diamond following the same principle that we had with the python 3 syntax conversion, ie. we will keep a list of expected failing tests in python 3 alongside the code.
A python 3 Travis job will not be available initially but may be added at a later date.

Work will proceed in feature branches off master which will be submitted as pull requests, reviewed, and merged in a timely fashion i.e. over a period of a couple of days.
This ensures we do not have long-running side branches leading to code conflicts and the need to merge from the master branch or rebase the feature branch.
The entire test suite is run on the iteratively updated master branch which means we avoid python 2 and - as the list of ignored tests shrinks - python 3 regressions.

Every feature branch will be concerned with a small number of topics. Initially these will be futurize stage 2 fixers, later these will be picked from remaining python 3 test failures. Commits in each pull request may fall into one of the following categories:

• automatic code conversion
• manual changes for test fixing commits
• idiomatic updates (eg. enumerate(), use of context managers, reliance on deprecated code, explicit list construction in places where generators would be more appropriate, ...)
• cleanup of immediate surrounding code (ie. cleaning campsite)
• improving code coverage, including adding tests
• flake8 compliance
• addressing of LGTM issues.

There is no required flake8/LGTM compliancy level for pull requests, and particularly at the start of the process it may be the case that pull requests will be merged with failing flake8 tests. As we refactor dxtbx and our code quality improves we should aim for more stringent flake8 compliancy and even consider enforcing more flake8 test categories via the pre-commit hook. Pull requests should not introduce any new LGTM issues, and ideally address existing issues in the touched files, although this may again not be practicable at the start of the process.

Benefits

At the end of the process dxtbx will be idiomatically modernized python 3 code that is fully python 2.7 compatible. This will improve the overall code quality, reduce technical debt and code maintenance cost, and reduce the barriers to entry for new or external developers.
The process will serve as a prototype for the upcoming work on the DIALS project.

Success criteria

• No test failures on Travis (Python 2.7, possibly 3.6 and 3.7)
• No test failures on DLS Jenkins (Python 2.7 and 3.6)
• Fewer than 10 remaining LGTM alerts (current: 121) with an A+ code rating (current: A)
• Fewer than 10 flake8 errors/warnings (current: 392)
• Code coverage at or above current levels (100% packages, 83% files, 83% classes, 62% lines, 45% conditionals)
• Delivery by mid-July 2019

Comments

Please discuss this process in the issue comments below. If this is agreeable we anticipate starting this work in the next week.

[Anthchirp]

this now exists. The regression test fails when tests that used to pass on Python 3 no longer do so. Python 2 testing remains in place as a separate build job.

[jbeilstenedmands]

I think this all sounds good, I like the sound of the separate feature branches - I'm hoping to learn a bit through this process as I'm not yet up to speed with all of the differences between python 2&3. For the python3-regression test branch, will we receive emails if this one fails or does it rely on checking the github page?

[Anthchirp]

I was going to reply "no, we can't have emails because Jenkins can't distinguish between expected and unexpected test failures", but pytest actually makes it really easy to keep a list of conditionally expected test failures, so I've just committed f3daa9c and we now should have Python3 test regression emails.

(The same logic will also apply when you run tests locally.)

[Anthchirp]

This work has now been completed.

• All tests pass on Python 2.7 and 3.6 in Jenkins and Travis
• 2 tests were disabled on Python 3 in 54113e8
  This is due to .pickle format files not being loadable. To fix this would require a substantial amount of time and changes in cctbx and libtbx for a use case that is unclear to me. If these files are actually used then someone will have to put the effort in, otherwise I suggest the relevant formats should be retired.
• We have achieved A+ rating on LGTM with 11 remaining warnings. This should go down to 3 very soon since
  • 1 was just fixed with f0f5b00
  • 3 will be fixed by XTCJungfrau: deduplicate code #61
  • 4 will be fixed by Refactor SMV format classes #63
• There are 51 remaining flake8 warnings, they break down as follows:
  • 28x E402 module level import not at top of file. These are caused by Python2/3 cross-compatibility work and should be ignored
  • 7xF401, 3x F403, 13x F405 - these are covered by Avoid * imports, explicitly declare exported symbols #62 and subject to the DIALS conventions in Add a CONTRIBUTING/standard/conventions document dials/dials#838
• Code coverage has slightly decreased in terms of files and classes (81%) and conditionals (44%). This is not entirely unexpected given that over the course of the work approximately 6% of dxtbx was removed (1500 lines), with the refactoring attention having been biased towards tested code rather than untested code.
• Project closed 10 day ahead of schedule.

Thanks to everyone involved.