Story #14939

Update crunchstat-summary to Python 3

Added by Tom Morris 13 days ago. Updated 3 days ago.

Status:
In Progress
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
03/08/2019
Due date:
% Done:

0%

Estimated time:
(Total: 0.00 h)
Story points:
-

Subtasks

Task #14940: Review 14939-crunchstat-summary-python3 -> 14939-crunchstat-summary-python3In ProgressTom Morris


Related issues

Blocks Arvados - Story #14532: Prepare for Python 2 sunsetting in December 2019New

Associated revisions

Revision 5d77a55a (diff)
Added by Tom Morris 12 days ago

Update crunchstat-summary to Python 3. Refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision cd33e7da (diff)
Added by Tom Morris 7 days ago

Make iterator Python 2 compatible. refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 22ed89be (diff)
Added by Tom Morris 7 days ago

Make log file reading Python 3 compatible. refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 031a3005 (diff)
Added by Tom Morris 7 days ago

Fix test file handling to support Python 3. refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 03fd14e6 (diff)
Added by Tom Morris 7 days ago

Add crunchstat-summary to Python 3 tests. refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 6bac55d2 (diff)
Added by Tom Morris 6 days ago

Update crunchstat-summary to Python 3. Refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision c1589536 (diff)
Added by Tom Morris 6 days ago

Add crunchstat-summary to Python 3 tests. refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 8a0bc9d2 (diff)
Added by Tom Morris 6 days ago

Update crunchstat-summary to Python 3. Refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision fdaf820a (diff)
Added by Tom Morris 6 days ago

Add crunchstat-summary to Python 3 tests. refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 7c3413ae (diff)
Added by Tom Morris 3 days ago

Update crunchstat-summary to Python 3. Refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 77c137e8 (diff)
Added by Tom Morris 3 days ago

Add crunchstat-summary to Python 3 tests. refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

Revision 44aa8df7 (diff)
Added by Tom Morris 3 days ago

Update crunchstat-summary to Python 3. Refs #14939

Arvados-DCO-1.1-Signed-off-by: Tom Morris <>

History

#1 Updated by Tom Morris 12 days ago

  • Status changed from New to In Progress
  • Start date set to 03/08/2019

#2 Updated by Tom Morris 12 days ago

  • Parent task deleted (#14532)

#3 Updated by Tom Morris 12 days ago

  • Tracker changed from Task to Story

#4 Updated by Tom Morris 12 days ago

  • Blocks Story #14532: Prepare for Python 2 sunsetting in December 2019 added

#5 Updated by Tom Clegg 10 days ago

LiveLogReader threads are crashing in the test suite, which seems to indicate two broken things:
  • Crashing threads don't cause the tests to fail, because it's Python and we don't explicitly catch exceptions in the thread and check/propagate them after calling join()... I guess we need to do that.
  • LiveLogReader.next() is still needed by Python 2

Should update run-tests.sh to test crunchstat-summary with Python 3 as well (add "tools/crunchstat-summary:py3" to pythonstuff like "sdk/cwl:py3"). It looks like there are some bytes-vs.-str things to sort out.

#6 Updated by Tom Morris 9 days ago

  • Target version changed from Arvados Future Sprints to 2019-03-13 Sprint

#7 Updated by Tom Morris 8 days ago

  • Target version changed from 2019-03-13 Sprint to 2019-03-27 Sprint

#8 Updated by Tom Morris 7 days ago

Tom Clegg wrote:

LiveLogReader threads are crashing in the test suite, which seems to indicate two broken things:
  • Crashing threads don't cause the tests to fail, because it's Python and we don't explicitly catch exceptions in the thread and check/propagate them after calling join()... I guess we need to do that.
  • LiveLogReader.next() is still needed by Python 2

Fixed the iterator and other thread crashers, but not the general thread exception catching.

Should update run-tests.sh to test crunchstat-summary with Python 3 as well (add "tools/crunchstat-summary:py3" to pythonstuff like "sdk/cwl:py3"). It looks like there are some bytes-vs.-str things to sort out.

Done.

Also fixed Python 3 log file reading, test failure caused by integer division diffs, and a couple of other things.

All tests pass locally in both Python 2 & 3 and CI job running here: https://ci.curoverse.com/view/Developer/job/developer-run-tests/1122/

#9 Updated by Tom Clegg 7 days ago

Could tests/test_examples.py use crunchstat_summary.command.GzipText instead of having its own copy of it?

Nit: usually kwargs are foo=bar but there are a couple of foo = bar here. meh

The rest LGTM, thanks.

#10 Updated by Tom Morris 6 days ago

I force pushed a new branch 14939-crunchstat-summary-python3 at 6cf111041f450c5d33665615ac915869c4279f9f which addresses your two comments as well as fixes:
  • resource leak warned by Python 3 (only)
  • deprecation warnings in Python 3

I also eliminated some unnecessary u'' additions and squashed some of the minor commits together and gave the UTF8 decoding wrapper class a better name.

If you could take a quick look to make sure it looks better, not worse, that'd be great.

#11 Updated by Tom Morris 6 days ago

I've put up another branch based on this one with a number of small fixes for #12026, #10570, & #14451 which all have Eric as the reviewer. Please coordinate with him to see if it makes more sense for one person to review all the changes as a batch.

#12 Updated by Eric Biagiotti 3 days ago

Calling .items() to iterate on a dict is inefficient in Python 2 according to the following: http://python-future.org/compatible_idioms.html#iterating-through-dict-keys-values-items. It recommends using viewitems(), though this will require a future.utils import. When working on the ACR, I ran into packaging problems after adding the future import. If you add it, its probably worth running the package building and tests locally.

Also a nit about an extra line of whitespace here: https://dev.arvados.org/projects/arvados/repository/revisions/14939-crunchstat-summary-python3/entry/tools/crunchstat-summary/tests/test_examples.py#L63

Looks like this change from cpu to cp was unintentional: https://dev.arvados.org/projects/arvados/repository/revisions/14939-crunchstat-summary-python3/entry/tools/crunchstat-summary/crunchstat_summary/webchart.py#L48

Also, do we use crunchstat-summary output to run cwl integration/conformance tests at all? If so, its worth running those as well.

#13 Updated by Tom Morris 3 days ago

Eric Biagiotti wrote:

Calling .items() to iterate on a dict is inefficient in Python 2 according to the following: http://python-future.org/compatible_idioms.html#iterating-through-dict-keys-values-items. It recommends using viewitems(), though this will require a future.utils import. When working on the ACR, I ran into packaging problems after adding the future import. If you add it, its probably worth running the package building and tests locally.

I think I'm willing to live with a few months of Python 2 only (unquantified) inefficiency in a non-performance critical section of code.

Also a nit about an extra line of whitespace here: https://dev.arvados.org/projects/arvados/repository/revisions/14939-crunchstat-summary-python3/entry/tools/crunchstat-summary/tests/test_examples.py#L63

Fixed.

Looks like this change from cpu to cp was unintentional: https://dev.arvados.org/projects/arvados/repository/revisions/14939-crunchstat-summary-python3/entry/tools/crunchstat-summary/crunchstat_summary/webchart.py#L48

Oops! Good catch. Fixed.

Also, do we use crunchstat-summary output to run cwl integration/conformance tests at all? If so, its worth running those as well.

No. I'm pretty sure there are no interactions with other components.

Also available in: Atom PDF